Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RelayStore.reset() #233

Closed
wincent opened this issue Sep 3, 2015 · 60 comments
Closed

Implement RelayStore.reset() #233

wincent opened this issue Sep 3, 2015 · 60 comments

Comments

@wincent
Copy link
Contributor

wincent commented Sep 3, 2015

The motivating use case here is clearing the cache on logout. We already have an internal task for this (t6995165) but I'm creating this issue for external visibility.

For now, the workaround is to do a full page refresh on logout. The long-term plan here is #559 - instead of resetting the store, applications will be create new instances of RelayEnvironment when necessary.

@steveluscher
Copy link
Contributor

This will be useful for the prototyping tools (#240). Right now, it's possible to paint yourself into a corner where you have cached results for a given field/call combination, but you've changed the underlying resolve method in the schema tab.

@quazzie
Copy link
Contributor

quazzie commented Sep 10, 2015

Great, need this :)

@clentfort
Copy link

Things that need to be touched just from a quick peek at the code:

  • In RelayStoreData.js: _records, _queuedRecords must be reset to {}, _recordsStore and _queuedStore must be reinitialized with the empty record-sets. _cachedRecords should stay as is, I assume. _queryTracker must be reinitialized. RelayStoreGarbageCollector should also be reinitialized if needed.
  • In RelayStore.js: queuedStore can no longer be cached.

The more interesting part: How should RelayStoreData announce this change to the rest of the system? Should Relay go back into a fetching-state after a call to reset?

@josephsavona
Copy link
Contributor

@clentfort Yeah, the tricky part isn't so much the resetting as it is deciding what to do if components are still mounted.

@clentfort
Copy link

@josephsavona: If we change some stuff we could use the infrastructure that is available for the garbage collector and simply error-out if any data is still subscribed to.
We could change the RelayStoreQueryResolver to emit events when new subscriptions are created/disposed, this would enable us to count the number of active subscriptions and not allow reset if there are any.
This would even allow us to decouple some things (i.e. we could make RelayStoreQueryResolver unaware of the garbage collector)!

@skevy
Copy link
Contributor

skevy commented Sep 26, 2015

Is there any work around for this that could be used on react-native? A page refresh isn't really an option there. :)

@josephsavona
Copy link
Contributor

@skevy The workaround for now in React Native is the brute-force equivalent of a page refresh - tear down the current JS context and start a new one.

@josephsavona
Copy link
Contributor

Here's what needs to change to make this happen:

  • Add Relay.Store.reset(): Implement an invariant check that there are no rendered Relay containers, no pending queries, and no mutations - only allow reset if this check passes.
  • Find every module that calls RelayStoreData.getDefaultInstance() and holds onto a reference to the result. Instead of caching the result in module scope, always call getDefaultInstance() when the instance is about to be used.
  • tests
  • documentation

I started a rough version of this at josephsavona@7fdb68d - feel free to use this as the basis of the above implementation. It implements the invariant check I mentioned above, but all the new methods need tests.

@josephsavona
Copy link
Contributor

cc @skevy @devknoll @taion @fson anybody interested? ;-)

@fson
Copy link
Contributor

fson commented Sep 27, 2015

Would it be reasonable for the RelayRootContainer to manage the
RelayStoreData instance?

My concern is that only exposing this low-level Relay.Store.reset() API, we
are leaving all the complexity of orchestrating the reset (tearing down the
components, resetting, rendering the components again) to the developer.

This could become quite complex to do, especially if data is fetched both before
and after the reset. For example resetting data on login/logout:

class App extends Component {
  state = { loginState: 'SIGNED_OUT' };
  handleLogin = () => {
    login().then(() => {
      this.setState({ loginState: 'TRANSITION' }, () => {
        Relay.Store.reset();
        this.setState({ loginState: 'SIGNED_IN' });
      });
    })
  };
  handleLogout = () => {
    logout().then(() => {
      this.setState({ loginState: 'TRANSITION' }, () => {
        Relay.Store.reset();
        this.setState({ loginState: 'SIGNED_OUT' });
      });
    })
  };
  render() {
    switch (this.state.loginState) {
      case 'SIGNED_OUT':
        return (
          <Relay.RootContainer
            Component={Login}
            renderFetched={(data) =>
              <Login {...data} onLogin={this.handleLogin} />
            } />
        );
      case 'TRANSITION':
        return null;
      case 'SIGNED_IN':
        return (
          <Relay.RootContainer
            Component={Profile}
            renderFetched={(data) =>
              <Profile {...data} onLogout={this.handleLogout} />
            } />
        );
    }
}

If the data would be tied to the lifecycle of RootContainer, one could simply
use the key property in the RootContainer to force it to be re-mounted when
the login state changes. Are there any disadvantages to that approach?

@taion
Copy link
Contributor

taion commented Sep 27, 2015

@fson

I like the idea of not having that global singleton Relay store.

Ignoring implementation difficulties for now, there are a couple of practical API considerations.

  • For things like modals and overlays and other dynamic content with data dependencies, when not associating a route to them, it's probably easiest to just set up a new RootContainer - this RootContainer ideally should share the same store as the top-level RootContainer; perhaps RootContainers can export the store as context and children RootContainers can try to use those first
    • The context approach requires things that use portals to use unstable_renderSubtreeIntoContainer, though... ideally anybody doing this is using something like react-overlays that deals with this for them
  • Same as above, but taking the naive routing approach @cpojer covers at https://medium.com/@cpojer/relay-and-routing-36b5439bad9, instead of using react-router-relay or something equivalent
  • Same as above, but for mocking .defer support with extra RootContainers
  • @skevy can comment on this, but I believe naive approaches with navigator on RN lead to multiple sibling RootContainers that would not be able to share state with this approach, and that actually keeping everything under a single top-level RootContainer requires quite a lot of work

@josephsavona
Copy link
Contributor

@fson Interesting idea. For background, we've found lots of use cases for having multiple <RelayRootContainer> instances that share data within a single application. For example, on React Native each screen within a navigation stack typically has its own root container, and if you're integrating Relay into an existing app you might have root containers for each small UI component that you convert to use Relay.

That said, you're absolutely right that a more complete solution is to integrate RelayRootContainer into the reset() lifecycle. Perhaps:

  • Allow reset() to be called while queries are pending and simply abort them.
  • If a reset occurs, RelayRootContainer immediately resets itself as it if was just rendered: it reissues queries for the route and shows the renderLoading indicator.
  • What to do about pending mutations is less clear.

@josephsavona
Copy link
Contributor

I like the idea of not having that global singleton Relay store.

@taion We agree. Earlier versions of Relay had way more global state, and we've been steadily refactoring to move this state into instance objects. The eventual goal is that you could configure a RelayContext-style object and pass it into the root container, and all query fetching, mutations, etc would then go through that context. This would obviously also help for server rendering.

@taion
Copy link
Contributor

taion commented Sep 27, 2015

@josephsavona

What about (optionally) separating that out from Relay.RootContainer entirely? The idea would be something like having a RelayContext component that exports the context via getChildContext.

Then each Relay.RootContainer could just try to get its relay context via its getContext (and optionally create a new one if there isn't one available already, to simplify normal use cases where you just have a single RootContainer up top).

This would work well for the standard RN use case, because you could just wrap the navigator in a RelayContext component and have the RootContainers share data. You'd still have to deal with unstable_renderSubtreeIntoContainer with dynamic modals rendered into portals for the web, but that's a much more minor issue.

The benefit of doing this would be that it might be possible to avoid an explicit imperative reset method, and just re-mount the RelayContext component, as @fson said.

@josephsavona
Copy link
Contributor

@taion Yes! That's basically the long-term vision. However, there's still a bunch of work needed to get there. The approach I outlined for Relay.Store.reset() is more meant as a useful stopgap until we have true RelayContexts.

@Globegitter
Copy link
Contributor

It would also be great to have the ability to invalidate certain entries of the cache. E.g. if we know that certain data can be updated from other sources or other users it would be great to be able to say, for example, after 10min invalidate this part of the cache and if the user re-visits that page requery the invalidated parts that are needed to display the current components.

@josephsavona
Copy link
Contributor

This issue will largely be addressed by #558, which makes all Relay state contextual (implements the RelayContext idea discussed above).

@Globegitter - Yeah, that would be cool. The API could be Relay.Store.invalidate(query), and the implementation would move any records or fields referenced by the query from the "fresh" data (RelayStoreData#_records) to "stale" data (RelayStoreData#_cachedRecords). This would cause Relay to refetch the information, without disrupting existing components that may be displaying the data.

@KyleAMathews
Copy link
Contributor

Instead of (or perhaps in addition to) invalidating it'd be nice to be able
to set expiration times. If relay is a cache it'd be nice to have normal
cache APIs available :-)
On Fri, Nov 13, 2015 at 10:51 AM Joseph Savona [email protected]
wrote:

This issue will largely be addressed by #558
#558, which makes all Relay
state contextual (implements the RelayContext idea discussed above).

@Globegitter https://github.com/Globegitter - Yeah, that would be cool.
The API could be Relay.Store.invalidate(query), and the implementation
would move any records or fields referenced by the query from the "fresh"
data (RelayStoreData#_records) to "stale" data (
RelayStoreData#_cachedRecords). This would cause Relay to refetch the
information, without disrupting existing components that may be displaying
the data.


Reply to this email directly or view it on GitHub
#233 (comment).

@josephsavona
Copy link
Contributor

@KyleAMathews Yup, we're looking into this. However, in early experiments storing the metadata required to record expiration times had a non-trivial impact on product performance.

@Globegitter
Copy link
Contributor

@KyleAMathews @josephsavona yep being able to set expiration times would be great as well. But yeah getting performance right is of course important as well ;)

@devknoll
Copy link
Contributor

Could expiration exist outside of core? Seems like if we exposed Relay.Store.invalidate then it should be fairly trivial to build on top of that.

I think it'd be really nice to keep the core fairly simple and break out relay-addons similar to React if we need to 👍

@KyleAMathews
Copy link
Contributor

Yeah I can definitely see supporting an expire command being
complicated/slow. Also perhaps not advisable as given the multi-faceted
nature of intertwined nature of graphql queries, an expire command could be
pretty blunt. Some views might not want its data invalidated from
underneath it. An invalidate command keeps the control on the view layer
which seems the right trade off for engineers/product owners to think about
things. Basically its a poor man's subscription :-)

A related question, could you invalidate on a timer while the view is
active and have the data refreshed underneath it?
On Fri, Nov 13, 2015 at 4:55 PM Gerald Monaco [email protected]
wrote:

Could expiration exist outside of core? Seems like if we exposed
Relay.Store.invalidate then it should be fairly trivial to build on top
of that.

I think it'd be really nice to keep the core fairly simple and break out
relay-addons similar to React if we need to [image: 👍]


Reply to this email directly or view it on GitHub
#233 (comment).

@josephsavona
Copy link
Contributor

A related question, could you invalidate on a timer while the view is active and have the data refreshed underneath it?

@KyleAMathews This is trivial to do today: use forceFetch. For example, to poll for updates on a query, use:

const query = Relay.createQuery(Relay.QL`query { ... }`, {var: 'foo'});
Relay.Store.forceFetch({query}, readyState => { ... });

@josephsavona
Copy link
Contributor

Could expiration exist outside of core?

@devknoll Good question. It isn't possible today without hijacking some internal methods. It could be an interesting experiment to try building it and see what hooks you need. Again though, the main consideration is perf.

@taion
Copy link
Contributor

taion commented Nov 13, 2015

👍 I've been very happy with using forceFetch for this pattern. It's much nicer than Flux equivalents since it's tied to the specific view getting refreshed.

@slorber
Copy link
Contributor

slorber commented Jan 4, 2016

@josephsavona hello

You mention a "forceFetch" method but I don't see it documented here is it normal? https://facebook.github.io/relay/docs/api-reference-relay-store.html

@steveluscher
Copy link
Contributor

@slorber
Copy link
Contributor

slorber commented Jan 6, 2016

thanks I wasn't looking at the good page!

@jardakotesovec
Copy link
Contributor

@KyleAMathews This is trivial to do today: use forceFetch. For example, to poll for updates on a query, >use:

const query = Relay.createQuery(Relay.QLquery { ... }, {var: 'foo'}); Relay.Store.forceFetch({query}, readyState => { ... });

Is there way to make it work as fatQuery, so I can refetch everything that is being tracked on some particular connection?

Reason why I just don't call forceFetch inside component that has full query is simply because I need to do it somewhere else.
Something like this:

const query = Relay.createQuery(Relay.QL`query {
    viewer {
        fonts @relay(pattern:true)
    }
}`,{});
Relay.Store.forceFetch({query});

@tuananhtd
Copy link

Kind of a related question how you guys handle after user logging in orther than full page reloading? Some views need to be added or updated

@josephsavona
Copy link
Contributor

@tuananhtd that's a great question for Stack Overflow :-)

@edvinerikson
Copy link
Contributor

I don't see any reason for this to be open anymore. Based on the discussion in #898 (comment) that concluded that this feature isn't needed now when the new environment api has landed (#558).

@josephsavona
Copy link
Contributor

@edvinerikson The question of how to reset the Relay store still comes up occasionally, so having this issue is a useful way for users to find the current status. I'll update the description to make the plan more clear.

@mattecapu
Copy link

How do you emulate reset() using the Environment API?

@joenoon
Copy link

joenoon commented May 1, 2016

This is how I'm doing it:

util.js

export let currentRelay = {
  reset: function () {
    let env = new Relay.Environment();
    env.injectNetworkLayer(appReplayNetworkLayer);
    env.injectTaskScheduler(InteractionManager.runAfterInteractions);
    currentRelay.store = env;
  },
  store: null
};

currentRelay.reset();

index.ios.js

import {currentRelay} from './util';

...

class App extends React.Component {

  ...

  doReset() {
    currentRelay.reset();
    this.forceUpdate();
  }

  doMutation() {
    currentRelay.store.commitUpdate(new MyMutation({}));
  }

}

use Relay.Renderer directly or create your own wrapper than passes environment={currentRelay.store}. see RelayRootContainer source

@josephsavona
Copy link
Contributor

I'm closing as this is now possible in the release branch. Stores cannot be reset, instead you can create a new instance of Relay.Environment and pass that to <Relay.Renderer environment={...} />.

@BlooJeans
Copy link

At the moment, using custom environments won't work with mutations. The patch is in the repo at b44fcb2 , but it hasn't been released, so you'll have to manually patch it in order to get custom environments to work with mutations. Once you're patched, you can do this.props.relay.commitUpdate(new MyMutation({ ...inputs }))

If you don't use the patched version, Mutations won't find cached objects correctly given in the inputs

Further reading: #1136

@jamesone
Copy link

@joenoon Have you got an example app which uses this to reset the environment? ^

@joenoon
Copy link

joenoon commented Feb 15, 2017

@jamesone actually what I do now is a bit different. I haven't tried changing the entire environment out in months, but I remember having problems getting it to work right in my app. Sounds like it might have been addressed since then.

Not sure if this will address your needs, but what I've been doing that seems to work pretty good for me is I have a simple mutation that by default has a fatQuery that will force the entire viewer to refresh. So when the app returns from the background I just issue this mutation, and anything open in the app will refresh with current data:

import Relay from 'react-relay';

export let ActiveMutation = class ActiveMutation extends Relay.Mutation {

  getMutation() {
    return Relay.QL`mutation {active}`;
  }

  getVariables() {
    const {fatQuery,...props} = this.props;
    return props;
  }

  getFatQuery() {
    if (this.props.fatQuery) return this.props.fatQuery;
    return Relay.QL`
      fragment on ActivePayload {
        viewer
      }
    `;
  }

  getConfigs() {
    return [
      {
        type: 'FIELDS_CHANGE',
        fieldIDs: {
          viewer: 'viewer',
        },
      },
    ];
  }

}

On the server side its just a mutation with this:

  ...
  outputFields: {
    viewer: {
      type: viewerType,
      resolve: () => ({})
    },
  },
  ...

@jamesone
Copy link

jamesone commented Mar 7, 2017

Why isn't there a command Relay.reset() which simply wipes EVERYTHING from the app?

@taion
Copy link
Contributor

taion commented Mar 7, 2017

Because it's much cleaner to just instantiate a new Relay.Environment() instead of trying to make sure you've cleaned everything on the global singleton Relay.Store.

And #233 (comment) no longer applies.

So just do the right thing and make a new environment.

@olso
Copy link

olso commented Jun 6, 2020

If it's for logout, just do window.location.reload()

@matt-dalton
Copy link

#233 (comment)

Because it's much cleaner to just instantiate a new Relay.Environment() instead of trying to make sure you've cleaned everything on the global singleton Relay.Store.

And #233 (comment) no longer applies.

So just do the right thing and make a new environment.

Sorry to dig up this old issue @taion. I've been following the approach you mentioned (creating a new store) for a while now, but an issue has just come to light.
When you logout this way, the Relay environment in the RelayEnvironmentProvider context is not updated. In theory we could manually update this using a hook, but this is difficult in certain places (e.g. relay network middleware, state containers)

The easiest thing seems to be to avoid creating a new store and instead clear the old one. Has anyone else experienced a similar issue and found a solution? Is everyone else updating their context using a hook/context when logging out?

@josephsavona
Copy link
Contributor

For logout, we recommend either reloading the application (window.location.reload()). An alternative is to create a new Environment (and store, and network), but this can be error-prone as you have to make sure that the previous environment isn't being used anywhere. If you're having trouble implementing the new environment approach, our recommendation is to just refresh the whole page.

We do not plan on providing a reset() method, as this just makes the problem worse: now if you have parts of the page still using the old environment they'll suddenly encounter missing data and possibly throw.

@matt-dalton
Copy link

Thanks for the response @josephsavona .

I'm using this in a React Native context. It's possible to reload the app in a similar way, but it looks super amateur and it's not typically how apps behave when you logout. Agree with the new environment approach being tricky.

Could this invalidateStore function work? Or would that not "reset" everything on the environment that we need?

@josephsavona
Copy link
Contributor

Note that invalidateStore() only marks data as stale, it does not clear data from the store. If you're in RN, then the best bet is to update the environment at the root context provider to ensure it updates the whole UI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests