Navigation Menu

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

Continuation of #698 - Redux time travel #701

Closed
migueloller opened this issue Sep 23, 2016 · 40 comments
Closed

Continuation of #698 - Redux time travel #701

migueloller opened this issue Sep 23, 2016 · 40 comments

Comments

@migueloller
Copy link

migueloller commented Sep 23, 2016

@tmeasday,

I went back and played around with time travel a bit. What seems to be popping out of the blue when I reset my store are APOLLO_QUERY_STOP actions. I'm not sure if these should be happening or not. Maybe you can shed some light on this?

Nevertheless, I'm realizing that time traveling a react-apollo app isn't really possible because data from the graphql higher order component gets called on componentDidMount. This means that as you try to reproduce each step, once you get to the step that would mount the GraphQL connected component, the queries are going to fire off, thus overlapping with the next actions one would manually dispatch while time traveling.

It's definitely possible to reduce the state from all the actions though. 😄

EDIT: Starts where this comment left off.

@migueloller migueloller changed the title Continuation of https://github.com/apollostack/apollo-client/issues/698#issuecomment-249065915 Continuation of #698 Sep 23, 2016
@migueloller migueloller changed the title Continuation of #698 Continuation of #698 - Redux time travel Sep 23, 2016
@tmeasday tmeasday self-assigned this Sep 23, 2016
@tmeasday
Copy link
Contributor

I went back and played around with time travel a bit. What seems to be popping out of the blue when I reset my store are APOLLO_QUERY_STOP actions. I'm not sure if these should be happening or not. Maybe you can shed some light on this?

I can't really see why that would happen. That action seems to only get fired by unsubscribing to the query -- are you maybe doing that?

Nevertheless, I'm realizing that time traveling a react-apollo app isn't really possible because data from the graphql higher order component gets called on componentDidMount. This means that as you try to reproduce each step, once you get to the step that would mount the GraphQL connected component, the queries are going to fire off, thus overlapping with the next actions one would manually dispatch while time traveling.

Are we talking about nested graphql components here? I could see that happening maybe, but I'm not totally understanding how that flow could come about otherwise (why would the component re-mount on query results otherwise?)

If it is the nested case, it's a bit of a doozy. I wonder if you have any ideas about how we could resolve this?

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

I can't really see why that would happen. That action seems to only get fired by unsubscribing to the query -- are you maybe doing that?

Ok, so if APOLLO_QUERY_STOP is fired when the query is unsubscribed then it would make sense for it to happen when the store is reset.

store is reset -> selected props change -> component with query unmounts -> query is unsubscribed -> ``APOLLO_QUERY_STOP action is fired

I didn't expect the query to be subsribed even after it was fulfilled. I thought after APOLLO_QUERY_RESULT or APOLLO_QUERY_RESULT_CLIENT came back the query would be unsubscribed.

Are we talking about nested graphql components here? I could see that happening maybe, but I'm not totally understanding how that flow could come about otherwise (why would the component re-mount on query results otherwise?)

It's not nested components. It's a react native app and there's navigation involved. When navigating from the login screen to the home page (connected with graphql higher order component) the query is automatically fired because apollo-react subscribes the query when the component mounts (I'm assuming this).

When I use Redux DevTools to play around with my store I start seeing the weird behavior. (e.g., APOLLO_QUERY_STOP gets called when a component is unmounted and queries get fired when a component gets mounted)

@migueloller
Copy link
Author

By the way, would you say nesting GraphQL components is an anti-pattern and that there should only be one top level query that gets everything and passes down the props as needed?

I originally thought I could take advantage of the cache and nest the components and the nested ones wouldn't have to do duplicate queries, but it turns out that doesn't work as well as one would expect.

@tmeasday
Copy link
Contributor

I didn't expect the query to be subsribed even after it was fulfilled. I thought after APOLLO_QUERY_RESULT or APOLLO_QUERY_RESULT_CLIENT came back the query would be unsubscribed.

RA needs to keep watching the query so later results to the query (from mutations or polling or whatever) can be reflected in your UI.

It's not nested components. It's a react native app and there's navigation involved. When navigating from the login screen to the home page (connected with graphql higher order component) the query is automatically fired because apollo-react subscribes the query when the component mounts (I'm assuming this).

Oh, so you are saying other redux actions you time-travel through are causing the component to get mounted? I guess that's more or less the same problem as the nested queries one if you think about it.

I wonder if dispatching actions from component lifecycle is an anti-pattern in Redux. It seems like you'd inevitably run into this problem when time-travelling. Although I've no real idea how you'd do it otherwise.

/me is going on a google journey now.

@tmeasday
Copy link
Contributor

By the way, would you say nesting GraphQL components is an anti-pattern and that there should only be one top level query that gets everything and passes down the props as needed?

No, I don't think so!

I originally thought I could take advantage of the cache and nest the components and the nested ones wouldn't have to do duplicate queries, but it turns out that doesn't work as well as one would expect.

Tell me more about this. I don't think these patterns have been completely settled yet.

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

Oh, so you are saying other redux actions you time-travel through are causing the component to get mounted? I guess that's more or less the same problem as the nested queries one if you think about it.

Yes, a case would be a simple nav push, like dispatch(NavigationActionCreators.push('home'). That will push an action that will cause a new scene to get rendered (i.e., component mounted) which will then trigger a GraphQL query to populate the feed in "home".

I wonder if dispatching actions from component lifecycle is an anti-pattern in Redux. It seems like you'd inevitably run into this problem when time-travelling. Although I've no real idea how you'd do it otherwise.

The only thing that comes to mind in this case would be to go extreme redux and simply dispatch a thunk (or start a saga) when the component mounts and then let it handle asynchrony from there and fire the actions that change the store as it goes. This still would happen automatically when the component mounts, though, so it really doesn't sole our problem. The only way to prevent this would be to have some sort of "time-travel mode" where thunks aren't added to the time travel list of actions... and actually, now that I think about it, I think that's the case.

So let me expand on this: If instead of react-apollo subscribing on componentDidMount it simply dispatched a thunk and the subscription logic was decoupled from the graphql higher order component (it happened in Redux land, triggered by the thunk), then it would be very easy to ignore this action when time traveling (time traveling ignores thunks by default), thus solving our issue with breaking time-travel. EDIT: Nevermind, the thunk would still get dispatched on mount, so all the actions dispatched by it would get dispatched. I'm not sure there is actually a way unless there's custom setting ("time-travel mode"?) that will entirely ignore certain actions (thunks maybe). REEDIT: Basically, if the logic is moved to Redux land, then implementing the "time-travel mode" would be very easy. If the logic is tied to the component then it's not possible.

Tell me more about this. I don't think these patterns have been completely settled yet.

Of course! 😄

So I'm building a production React Native app that uses GraphQL and Apollo. I'm running into a lot of real-world use cases so I wouldn't mind sharing what I've come through/learned. For example, I know you're still missing a Relay example in the docs for pagination in react-apollo. I'm doing queries like that myself so perhaps I could contribute to the example once I finish the busy task of getting this shipped.

Anyway, to answer your question. Sometimes nested components can get the necessary props from the parent but other times the queries are completely different and the caching system won't help you. For example, if you have a query called comments that returns [Comment!]! and another one called comment whose signature is comment(id: ID!): Comment, there is no way of reconciling the responses of queries to comments with those of comment. Even if you use dataIdFromObject, that's only good for updating the cache, there is no way to tell apollo, "hey, all the ids that were returned from comments actually map to ROOT_QUERY.comment(id: $id), where $id is the one we just got from dataIdFromObject.

Another thing that I haven't looked much into is that I think that the nested components mount first (before their parent components), so it becomes awkward to fetch a lot of fields from the nested components simply to cache them. Anyhow, as I keep building the app I'll keep finding better ways of approaching each issue.

I hope this is helpful.

@tmeasday
Copy link
Contributor

So let me expand on this: If instead of react-apollo subscribing on componentDidMount it simply dispatched a thunk and the subscription logic was decoupled from the graphql higher order component, then it would be very easy to ignore this action when time traveling (time traveling ignores thunks by default), thus solving our issue with breaking time-travel.

Is that true? (That TT ignores thunks). What is the mechanism for that? I wonder if there's an equivalent approach we can take.

So I'm building a production React Native app that uses GraphQL and Apollo. I'm running into a lot of real-world use cases so I wouldn't mind sharing what I've come through/learned. For example, I know you're still missing a Relay example in the docs for pagination in react-apollo. I'm doing queries like that myself so perhaps I could contribute to the example once I finish the busy task of getting this shipped.

That'd be awesome!

For example, if you have a query called comments that returns [Comment!]! and another one called comment whose signature is comment(id: ID!): Comment, there is no way of reconciling the responses of queries to comments with those of comment

That makes sense; there's not really any way around that. I do wonder what the use case is for a separate query for the comment however (I am imagining a list of comments, but maybe I'm off base here?).

Another thing that I haven't looked much into is that I think that the nested components mount first (before their parent components)

Wouldn't the nested component depend on the result of the parent component? I would have thought if that is the case they would have to wait on the parent's query. But maybe we need to be more concrete in the example..

@migueloller
Copy link
Author

Is that true? (That TT ignores thunks). What is the mechanism for that? I wonder if there's an equivalent approach we can take.

I'm sorry for using the wrong nomenclature here. The Redux DevTools store enhancer will only track dispatched plain actions (POJOs), meaning that thunks (functions) will not get recorded. I'm not 100% on this but I'm pretty sure I read about it a while ago. I tried to find the source but no luck. The idea is similar to putting redux-logger as the last middleware so that it doesn't pick up thunks. In this case, instead of forcing the developer to put it last, the middleware doesn't record if typeof action === 'function'. Honestly, I've got to do more research on this.

That makes sense; there's not really any way around that. I do wonder what the use case is for a separate query for the comment however (I am imagining a list of comments, but maybe I'm off base here?).

Imagine having a feed of items, lets say tweets. The query to get the feed is analogous to comments and the query to get the details of a tweet is analogous to comment. In GraphQL you can get all the details from comments simply by specifying the fields but it would be nice to separate concerns and have the Feed component only query all the IDs of each tweet and then have each individual FeedItem component query the details of the tweet. To be honest though, you probably wouldn't do this. I was doing this initially in our app's feed and it was rendering weirdly (some requests came earlier than others causing rendered components to jump around). Also, you kind of only want to do 1 trip to the server, even if it means a much bigger response that takes longer. Ultimately it depends what you want to optimize for. I'm looking forward to the release of @defered to be able to defer some parts of the query and render the most important parts first.

Wouldn't the nested component depend on the result of the parent component? I would have thought if that is the case they would have to wait on the parent's query. But maybe we need to be more concrete in the example..

You're right, the child component probably the parent to be there to be able to mount in the first place.

@migueloller
Copy link
Author

Forgot to mention, I edited my comment above. Not sure if you saw the edits.

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

@tmeasday,

By the way, if you want me to, I'd be happy to go into more detail into what I mean by having the query logic live in the Redux side.

@tmeasday
Copy link
Contributor

Re: nested queries:

I think the better approach here is to use a fragment in this case (where you want to separate the concerns about the structure of the comment query). Then the feed can grab the fragment off the comment and include it in the query. Our fragment support is pretty raw and we don't really have great patterns for this but it's something we actively want to explore and figure out a solution to.

Probably something less intense than what Relay does is where we'll end up.

By the way, I'd be happy to go into more detail into what I mean by having the query logic live in the Redux side.

The part I can't figure is how if your component dispatches in its lifecycle callbacks (via thunks or otherwise) how you could avoid extra actions (forget network requests for a moment) occurring when the component is rendered during TT, unless there's some special mechanism where Redux stops dispatching or something.

I too need to do a lot more research in this area but it's something I do want to look at in a lot more detail soon. I'll report back as I figure stuff out.

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

I think the better approach here is to use a fragment in this case (where you want to separate the concerns about the structure of the comment query). Then the feed can grab the fragment off the comment and include it in the query. Our fragment support is pretty raw and we don't really have great patterns for this but it's something we actively want to explore and figure out a solution to.

You're right! Fragments are exactly what I need.

Probably something less intense than what Relay does is where we'll end up.

Have you seen what lokka does for fragments?

The part I can't figure is how if your component dispatches in its lifecycle callbacks (via thunks or otherwise) how you could avoid extra actions (forget network requests for a moment) occurring when the component is rendered during TT, unless there's some special mechanism where Redux stops dispatching or something.

You're exactly right about Redux having to stop the dispatching. Basically, "time-travel mode" could be activated by dispatching an action ACTIVATE_TIME_TRAVEL. Once this is dispatched, the "time-travel" middleware could kick in, at which point only POJOs can be dispatched. If a thunk is dispatched it is ignored (maybe still logged but ignored). This way one can time travel without worrying about side effects resulting from asynchrony or thunks. When you're done you can simply dispatch DEACTIVATE_TIME_TRAVEL. Makes sense?

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

I know this is completely outside the scope of Apollo now, but I'm enjoying the discussion! 😄

@tmeasday
Copy link
Contributor

You're exactly right about Redux having to stop the dispatching. Basically, "time-travel mode" could be activated by dispatching an action ACTIVATE_TIME_TRAVEL. Once this is dispatched, the "time-travel" middleware could kick in, at which point only POJOs can be dispatched. If a thunk is dispatched it is ignored (maybe still logged but ignored). This way one can time travel without worrying about side effects resulting from asynchrony or thunks. When you're done you can simply dispatch DEACTIVATE_TIME_TRAVEL. Makes sense?

If I was implementing this from scratch I would do the first half of what you said but when replaying actions attach an extra __fromTT__: true field; then I would add a middleware that just dropped actions on the floor that didn't have that field. Maybe that's what redux dev tools does? I should look...

@migueloller
Copy link
Author

migueloller commented Sep 23, 2016

Redux DevTools actually doesn't do that. Maybe it's something they should implement? It sounds like that should be the functionality out of the box given that when you're time traveling you wouldn't want to dispatch extraneous actions.

@migueloller
Copy link
Author

Pinging @zalmoxisus and @gaearon.

@zalmoxisus
Copy link

zalmoxisus commented Sep 24, 2016

If I was implementing this from scratch I would do the first half of what you said but when replaying actions attach an extra fromTT: true field.

The extension provides a getMonitor > isTimeTraveling function you can use for that purpose. You can add a __fromTT__ parameter via a middleware like so or you can just use a global variable for debugging. This API is still experimental so any suggestions are welcome.

UPDATE: I just read that it's about React Native. So you're probably using Remote Redux DevTools?

Basically, "time-travel mode" could be activated by dispatching an action ACTIVATE_TIME_TRAVEL. Once this is dispatched, the "time-travel" middleware could kick in, at which point only POJOs can be dispatched. If a thunk is dispatched it is ignored (maybe still logged but ignored). This way one can time travel without worrying about side effects resulting from asynchrony or thunks. When you're done you can simply dispatch DEACTIVATE_TIME_TRAVEL. Makes sense?

Yes, we want to implement something like this to lock all the changes while time travelling. Would that help?

@migueloller
Copy link
Author

migueloller commented Sep 24, 2016

@zalmoxisus, thanks for chiming in!

I think that exposing isTimeTravelling is great. It's very easy to implement middleware that ignores any actions that aren't from time travel.

I guess my question is, do we want the dev to handle this or should there be a switch in the DevTools monitor that when on will simply ignore any actions not triggered by the monitor?

What I mentioned above, regarding ignoring anything not a POJO, doesn't really work well because a POJO sent in componentWillMount can still trigger side effects (i.e., via middleware). The correct approach would be to simply ignore any action that doesn't come from the monitor, given a certain predicate (i.e., isMonitorAction)

EDIT: And it turns out you already expose such predicate! 😄

@migueloller
Copy link
Author

migueloller commented Sep 24, 2016

@zalmoxisus,

Also, I really love the work you've been doing to handle errors remotely and in realtime as they happen in production. It would be impossible to do this though without suppressing side effect actions from componentWillMount, other middleware, etc.

EDIT: For the remote user that's getting their app debugged, once time travel mode kicks in, they basically wouldn't be able to do anything in the app (as long as all app interactions are handled by Redux).

@zalmoxisus
Copy link

@migueloller, thanks for the feedback!

I guess my question is, do we want the dev to handle this or should there be a switch in the DevTools monitor that when on will simply ignore any actions not triggered by the monitor?

Redux DevTools enhancer knows nothing about action creators. It recomputes only reducers. That said, if the dev calls an action creator, for example from componentDidMount, we can prevent only invoking its reducers. Anyway we could prevent side effects from middlewares like redux-saga. That's exactly what we want achieve in zalmoxisus/redux-devtools-instrument#8. The initial idea was just to froze dispatching till unlocking it, but I guess it will not suit you use case, so better we'll cancel dispatching new actions at all while it's locked.

@migueloller
Copy link
Author

migueloller commented Sep 24, 2016

@zalmoxisus,

Yes, being able to just drop all actions that don't come from the monitor would be awesome! It would allow for time travel without having to worry about actions springing up from componentWillMount or other middleware that dispatch actions (redux-saga is one of these, but basically any middleware could do this).

Thanks for the prompt responses!

@zalmoxisus
Copy link

@migueloller, I've just published redux-devtools-instrument@1.2 and remote-redux-devtools@0.4.9, which support this feature. We still need to update the UI (add a button) on remotedev-app (and the extension), however you can get it work just by calling in your app:

store.liftedStore.dispatch({ type: 'LOCK_CHANGES', status: true });

To unlock changes:

store.liftedStore.dispatch({ type: 'LOCK_CHANGES', status: false });

Please let me know whether it helps.

@tmeasday
Copy link
Contributor

Seems pretty great!

My naive thought (and I'll admit I haven't spent a lot of time thinking about this), was that when the user time travelled, the TT mechanism would automatically lock dispatches until rendering is done (maybe a small timeout to be safe).

Then if the component sticks to dispatching single actions (which could be picked up by something like redux saga), or thunks, then it will be "locked out" during React's component lifecycle and everything should work.

If the component doesn't follow that pattern, and instead initiated network calls / something else that dispatched actions after timeouts (this is what Apollo is doing right now I think), then it wouldn't work; but I suspect the above way is more idiomatic Redux anyway, right?

@migueloller
Copy link
Author

@zalmoxisus, awesome! Definitely helps. Now I'm just waiting for the button in the monitor so that I don't have to use the dispatcher. 😉

@tmeasday, yeah, it looks like blocking all actions while time traveling is the best way.

Thanks all for the great discussion. I'm satisfied with the outcome and am going to close the issue. 😄

@tmeasday
Copy link
Contributor

@migueloller is there still an issue that Apollo dispatches actions in the "wrong" way and this mechanism won't (for instance) stop network requests when you are TT-ing?

@migueloller
Copy link
Author

@tmeasday, nope. Apollo never did anything wrong really. I just got confused with actions getting fired on componentDidMount and thought it was an Apollo issue, when really it's unavoidable for any action that is dispatched as the app goes along (either a component mounts, a user clicks a button, something times out, etc.). Will the changes @zalmoxisus made time travel will simply ignore all actions so it's all good.

The one thing that could be "wrong"/annoying is all the APOLLO_QUERY_STOP actions, But I believe those are dispatched only when a subscribed component dismounts and the subscriptions should never happen anyway when time traveling.

Does dispatching APOLLO_QUERY_STOP actually do something or changes some behavior or is it purely for logging purposes?

@tmeasday
Copy link
Contributor

tmeasday commented Sep 25, 2016

@migueloller but am I not correct in saying when a graphql container renders, the following will occur?:

  1. Immediately:
    • APOLLO_QUERY_INIT is dispatched (this will get squashed if the store is "locked")
    • a network request is fired off (this will still happen)
  2. Sometime later (after which the store may be unlocked)
    • network request returns, APOLLO_QUERY_RESULT fires

Aside from the unnecessary network requests, it seems like that APOLLO_QUERY_RESULT could be an issue.

I would have thought the APOLLO_QUERY_STOP would be OK -- it fires immediately when the component is unmounted, which would be during the "lock" period.

Does dispatching APOLLO_QUERY_STOP actually do something or changes some behavior or is it purely for logging purposes?

It removes the query status from the store. In the future you could imagine it might remove the query results or mark them to be GC-ed.

@migueloller
Copy link
Author

@tmeasday and @zalmoxisus,

Ok, so there are 2 different things we're talking about here.

The first one is what @zalmoxisus just mentioned which is to be able to lock all changes to be able to work on some feature. From seeing this line though, it looks like it's not made for time travel, but more to simply lock the entire store so that nothing happens. So it looks like this specific change won't solve our problem.

The second is one is what @zalmoxisus had mentioned in a previous comment regarding the isTimeTraveling field from the object returned by getMonitor() here. That does look like it would help with what we're trying to accomplish. To do this though, we would need to implement a custom tool that would check if the isTimeTraveling is true, and if it is then to ignore all actions that aren't from the monitor.

@zalmoxisus, do you see how these are 2 different use cases? I could even potentially see 2 different buttons in the monitor for them. One to drop all actions and another to drop all actions that don't come from the monitor.

@tmeasday, to answer your question:

If that is the way that Apollo currently handles it, then yes, it will interfere with time travel. But this will only happen if the store is unlocked (i.e., accepting non-time-travel actions) when the result comes back. This is unlikely. It is possible to make it 100% foolproof though, doing it with super idiomatic Redux. To make it so, it would be necessary to have a custom Apollo middleware that initiates the network process. See redux-api-middleware for an example. With middleware like that, the action that calls the API would never be dispatched, so the network request wouldn't be made and the result wouldn't be a problem.

I think that if Apollo implemented something like this internally it would definitely be cleaner. Then basically, what a connected GraphQL component does is dispatch an action when it mounts, it doesn't even have to worry about unsubscribing when it unmounts because all of that happens internally in Redux land.

@migueloller migueloller reopened this Sep 26, 2016
@zalmoxisus
Copy link

@migueloller, could you please elaborate why LOCK_CHANGES doesn't solve the problem? We want to prevent side effects not only for time travelling, but also for skipping (canceling) actions, for importing states, and most important for hot reloading (which also invokes recomputing all actions).

@zalmoxisus
Copy link

zalmoxisus commented Sep 26, 2016

I could even potentially see 2 different buttons in the monitor for them. One to drop all actions and another to drop all actions that don't come from the monitor.

In case of time travelling, LOCK_CHANGES does exactly this (drop all actions that don't come from the monitor) because it doesn't recompute anything. In other cases it also allows recomputing actions that was already dispatched.

@zalmoxisus
Copy link

zalmoxisus commented Sep 26, 2016

DevTools enhancer is the last in the compose, and all the middlewares are invoked before it. We need the second enhancer at the beginning. I've just published remote-redux-devtools@0.5.0-alpha, where you can use composeWithDevTools like in the example. It should drop actions before invoking any middlewares when it's locked. Please give it a try.

However, I guess you want to prevent invoking network requests not from middlewares (as it would be in case of redux-saga), but from the subscribed functions, which I'm not sure how to handle better.

Update: What if we'll expose a global variable like window.__REDUX_DEVTOOLS_IS_LOCKED__, which you could check (process.env.NODE_ENV === 'production' || typeof window !== 'object' || !window.__REDUX_DEVTOOLS_IS_LOCKED__) before invoking network requests? The reason why we cannot make isTimeTraveling global is that it's just for a specific instance (store), but here we're locking all the stores.

@migueloller
Copy link
Author

@zalmoxisus,

I apologize, I thought that LOCK_CHANGES would drop EVERY action, including the ones from the monitor. If actions from the monitor aren't dropped then it works perfectly.

@tmeasday
Copy link
Contributor

tmeasday commented Sep 27, 2016

@zalmoxisus I'm finally caught up on this. I tried using the store.liftedStore.dispatch({ type: 'LOCK_CHANGES', status: true }); etc above but it didn't seem to quite work as I thought it would.

I created a really simple example that tries to simulate how apollo-client might behave to help with this [1]. The basic flow is

  1. Page has two panes, when left pane is visible a "subscription" is inited in a thunk (in componentDidMount).
  2. When you switch to the right pane, the subscription is torn down via another thunk (in componentWillUnmount).

I definitely see issues when I time travel around in that because replaying actions makes the left pane appear and disappear, and new subscription and teardown actions occur.

Locking does not appear to stop that happening -- it does however seem to lock the UI.

Is this how you imagine a dev-tools compatible Redux based network layer might operate? Is there a better way to think about it?

[1] Code is here, there is a bit of other react-persist stuff in there but I think it's not affecting this: https://github.com/tmeasday/test-redux-app

@zalmoxisus
Copy link

@tmeasday, thanks for the example!

I've just tried it, and see the issue with time travelling, but everything works fine when it's locked (after store.liftedStore.dispatch({ type: 'LOCK_CHANGES', status: true });): no actions dispatched and no running subscribe thunk in the console.

Am I missing something?

@tmeasday
Copy link
Contributor

@zalmoxisus you know what, you're right! I don't know what I did before, but when I tried again it worked exactly as I hoped. Great! I think we are on the same page then.

So would your opinion be that driving network operations via thunks is the way to go? It seems like it to me.

@zalmoxisus
Copy link

zalmoxisus commented Sep 27, 2016

Thunks would be ok, as well as other middlewares. You could also have your custom middleware like in the real-world example. Locking will block all middlewares and store enhancers.

In case of having network request from store.subscribe or from the connected React components, one should prevent side-effects by himself. We'll expose window.__REDUX_DEVTOOLS_EXTENSION_LOCKED__ for that.

In future we could autolock the store while timetravelling.

Thanks for investigating this!

@zalmoxisus
Copy link

Locking was implemented in Redux DevTools Extension and in Remote Redux DevTools. Here's a blog post with more details.

@zalmoxisus
Copy link

zalmoxisus commented Oct 7, 2016

@tmeasday, thinking over, I don't think we should force users to add redux-thunk middleware, better to execute side effects right from (new ApolloClient()).middleware() for specific action types:

function clientMiddleware(store) {
  return next => action => {
    const result = next(action);
    // ...
    if (action.type === 'SUBSCRIBE_REQUEST') {
      // network requests or other sideffects
      // here we can dispatch also: next({ type: 'SUBSCRIBE_SUCCESS' })
      // or: next({ type: 'SUBSCRIBE_FAILED' })
    } else if (action.type === 'TEARDOWN_REQUEST') {
      // network requests or other sideffects
    }
    return result;
  };
}

And from the component:

  componentDidMount() {
    this.props.dispatch({ type: 'SUBSCRIBE_REQUEST' });
  }
  componentWillUnmount() {
    this.props.dispatch({ type: 'TEARDOWN_REQUEST' });
  }

@migueloller
Copy link
Author

@zalmoxisus,

Yes! That is the way to go.

@stale
Copy link

stale bot commented Jul 29, 2017

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants