Prevent infinite loops when state is updated before the actual history change is fired

[mariocasciaro]

I dispatch some actions to redux during the history.listenBefore hook, this causes multiple state changes before window.location is actually updated and before history.listen is fired. This causes an infinite loop, because redux-simple-router recognizes a route change at every state change, since window.location is still not updated, which in turn triggers the history.listenBefore hook, which triggers new state changes, and so on. This PR fixes this issue by making sure not to re-invoke pushState unless the previous route transition is completed.

[jlongster]

I'm curious; you are dispatching an UPDATE_PATH action in listenBefore, but that hook was fired by an existing transition. So you are trying to change the URL in the middle of an existing transition? Are you checking if the user is logged in and redirecting or something like that? Tell me more about your use case.

I only ask because I want to help people do it the way that react-router is built for, and sometimes it's not obvious what the correct way is. What would happen if you did a pushState inside of listenBefore? Seems like it would also be an infinite loop. Think of the UPDATE_PATH action as literally a pushState call. If it wouldn't work in react-router, why should we make it work here? There might be a better pattern when using react-router.

[mariocasciaro]

I'm not sending any UPDATE_PATH action during a transition. Here's what happens in short:

• current window.location is '/here'
• button click -> updatePath('/there')
• history.listenBefore() is called, here I do some data retrieval before the new route is actually rendered, to avoid flickering UI or to show loading indicators
• ANY state change (not necessarily to the routing, simple-redux-router listens to any change in the state) triggered in the listenBefore() hook will cause simple-redux-router to send a new pushState because store.routing = '/there' (<- the new state) while window.location is still '/here' because the transition is not completed yet. The new pushState will result in a new call to history.listenBefore()
• The last two steps result in an infinite loop

[mariocasciaro]

I updated the solution to also consider the case when using the browser navigation buttons (which were creating the same issue). This solution is even simpler, we simply check if there was any update in the routing path every time there is a state update.

[jlongster]

Oh wow, that's really cool. I actually was not aware of listenBefore, that's a good way to block changes if you need to. There's a couple things I want to think about more, and I'm at work, but I will respond in more depth later today. Thanks!

[mariocasciaro]

Ok, perfect. FYI I actually took your fork of react-redux-universal-hot-example as starting point and improved it a bit by adding data loading through fetchData() on history.listenBefore and data loading through fetchDataDeferred on history.listen. This way the two methods will behave exactly how they should when defined on a component.

[jlongster]

Cool! Did you put it somewhere? I can pick up where you left off. Been meaning to polish it off.

[mariocasciaro]

I've got all code integrated in a project at the moment, need to find some time to put everything back into your react-redux-universal-hot-example. Would be nice if we could merge those changes back into the main repo. I'll try to work on it tomorrow.
What about this PR by the way?

[jlongster]

Why do you pass true here? This flag disables all future router updates (only the redux state will change). It doesn't just disable it once.

[mariocasciaro]

When history.listen is called the transition to the new route already happened, so a new pushState is redundant. We only need an update to the state. Route changes triggered externally through actions/reducer will reset the flag (unless set explicitly in the action)

[jlongster]

Won't getRouterState().path and locationToString(location) be equal here then if the transition already happened? What's the difference between this workflow and the normal one where you update the store which triggers a router update? The only difference is that this history.listen event will be delayed, but it should end up in the same place: the values are equal here.

Just trying to fully understand what I'm merging in.

[mariocasciaro]

The idea is that you keep the state up-to-date by listening to the history (lines 49-55), the history update might come from a direct call to history.pushState() or from hitting the back button of the browser. This causes the saved routing state (state.routing) to be out of sync with the location. The purpose here is to keep the state updated as soon as we detect a transition.
The other way around is when we detect a change in state and we need to trigger a transition (lines 58-71), in this case somebody explicitly requested a route change by dispatching the UPDATE_PATH action and we need to react by calling history.pushState().
Please let me know if that makes sense

[jlongster]

Right, but that sounds like exactly how it has always worked. What is different now that noRouterUpdate is needed?

When the store listener is called, the paths will be the same already and nothing will happen. (I am traveling today so I won't be too responsive.)

Can you possibly remove this true, run it locally and tell me exactly what breaks?

[mariocasciaro]

You are actually right, it should't change anything because of the check currentState != currentLocation, that true doesn't add anything in terms of functionality. I added it for clarity, to make it clear that the action is not meant to update the route, but just the state. I'll remove that true if you think is redundant.

[mariocasciaro]

OK, I added the comment.

[mariocasciaro]

So, as promised I updated your react-redux-universal-hot-example with the prefetching pattern we were talking about. PR here: jlongster/react-redux-universal-hot-example#4

[jlongster]

Oh sweet, thanks for the PR!

[jlongster]

Thanks. I may even remove the noRouterUpdate flag in the future; I think it's confusing because it disables all future router updates. I want to re-think it at some point. So that's why I don't think it clarifies things much more; the syncing process critically depends on the state "resolving" to the same thing so that the listeners stop calling each other, and I want people to understand that the most.

[jlongster]

@mariocasciaro Actually, you may be right about using the noRouterUpdate flag to handle flow. I've been fixing some other issues about when we trigger the router (including this one) and that might be the most elegant solution. Still, I'd like to do it as a followup.

I'm going to open a new issue soon about refactoring that code.