Fix double update on history.pushState

[kimjoar]

Not the nicest tests or the nicest implementation, but a suggestion for how to handle the double updating reported in #108.

(Of course, if we end up going in this direction I'll add in some comments and tune the tests)

Anyone else got ideas that don't break any of the other tests? I'll try to play around with it a bit more too.

[kimjoar]

Stopping those cycles ain't easy — too many edge cases ;)

[jlongster]

We shouldn't need this code, this should have done it:

       // Also set `lastRoute` so that the store subscriber doesn't
       // trigger an unnecessary `pushState` on load
       lastRoute = initialState

The initial action that is dispatched through the system should set the lastRoute variable so the changeId check should not pass. Can you look into why it doesn't?

I'd prefer to keep the syncing as possible (I'm sure you do too) and if we do something like this we might as well look into changing how the whole avoidRouterUpdate thing works.

[tomatau]

This PR doesn't solve the issue for me as reported in #108

edit: although, this behaviour I'm seeing is when both reduxSimpleRouter is integrated as well as a route specific dispatch is invoked within onUpdate... again, this behaviour doesn't happen when pushPath or updatePath are used.. it's only when a Link or IndexLink is used to navigate.

[kimjoar]

@jlongster This isn't related to initialState, it's related to pushState/replaceState. Without my fix the test failure is:

Expected [ '/', '/bar', '/bar' ] to equal [ '/', '/bar' ]

I.e. bar is applied two times.

EDIT: Btw, this is every time. If you history.pushState two times:

Expected [ '/', '/bar', '/bar', '/baz', '/baz' ] to equal [ '/', '/bar', '/baz' ]

[kimjoar]

@tomatau So it still gets double applied? Could you create a small reproducible example?

[tomatau]

@kjbekkelund I've edited my comment to explain the more of the specifics around the problem.

The complete example is linked in the OP of the original issue.

[kimjoar]

@jlongster The problem with avoidRouterUpdate is that we needed a change to handle DevTools, but this change ended up with another side-effect (this double update). But removing changeId is difficult because of listenBefore etc. So I think it ends up being somewhat complex either way. We could of course just say that we don't support DevTools, that would simplify the code.

[kimjoar]

Hm, I'll play around with it and see what I find out

[jlongster]

@kjbekkelund the code I mentioned before was there to fix pushState / replaceState. It sets the lastRoute variable and then fires the initPath action, and in the store.subscribe function it checks changeId of lastRoute and the current route and since it's the same it does not call another pushState.

Just explaining the intent of the code; I haven't looked deeply at the problem here yet.

[kimjoar]

@jlongster Yep, but it only sets it initially. But if you later history.pushState it does not change the changeId, but it changes the path, so this check

if(lastRoute.changeId !== routing.changeId ||
   !locationsAreEqual(lastRoute, routing)) {

returns true. So avoidRouterUpdate is not enough to stop the cycle because we check two things: the path and the changeId (that's the change we needed because of DevTools). Looking at potential changes now.

[jlongster]

So avoidRouterUpdate is not enough to stop the cycle because we check two things: the path and the changeId (that's the change we needed because of DevTools). Looking at potential changes now.

Oh, I see. I feel like this PR might work but we should think about removing or changing the other checks; seems like if we do this it might stop the cycle in most cases?

I also had another idea before: the new routing state, when changed, is always a different instance. So we could also just do if(lastRoute !== routing) { ... } but I think the problem was how to implement something like avoidRouterUpdate. Maybe that combined with this PR would work. Or a simple avoidRouterUpdate boolean. I dunno.

Do you think you could look into the above? If changeId isn't really working out we can just remove it altogether if something else is simpler.

EDIT: fixed naming of avoidRouterUpdate

[jlongster]

I'll be more active next week, as I'll be back from a work trip! We can hopefully move to rackt then too.

[kimjoar]

@jlongster We already have problems with avoidRouterUpdate it seems: #118 :/ (The unit test succeeds, but there wasn't any "integration" test)

[jlongster]

I don't think that's a systemic problem of avoidRouterUpdate but just how that whole if check is implemented in the store subscriber. But if it works to keep a local variable and track the code flow that way, that's fine.

Last time I tried that it was really hard to debug problems though.

[bman654]

Isn't the whole problem that Router is listening to the history object and updating its internal state (which triggers a re-render) and redux-simple-router also listens to the history object and generates an action to update the Redux store, which also updates the history object, which triggers another Router update.

It seems like the root cause of all the cycles is that there are 2 sources of truth. Couldn't there be a different solution? Give Router a "ReduxHistory" object that is initialized from the real History object. When Router writes to the ReduxHistory, it generates redux actions (instead of updating the real history object directly). When Router listens to the ReduxHistory object, the listen subscription is delegated to the real History object. When the redux store updates, the changed state is pushed into the real history object, which will then trigger Router to update.

Net effect is now there's a single source of truth and no cycles to fix. Is there something wrong with this approach?

[kimjoar]

Hi @bman654, thanks for the input!

Are you talking about react-router here? I'm not sure I understood the flow here. Today we don't do anything related to react-router itself. We only rely on history and redux.

As far as I can tell what you describe is basically what the library does today, but without going through react-router. (It might be that would help here, so it would be great to get more information about how you see this working)

Could you draw up how this would work for these two cases:

1. The user changes the URL. We'll then have a new location and need to keep the store in sync. (We do this now by listening for a new location in history.listen and generate redux actions to update the store.)
2. The user sends a pushPath action. We'll then have an updated state in the store and want to keep the location in sync. (We do this now by listening for a store update and then update the history).

Today, 1 generates redux actions, so it triggers 2, and 2 updates the URL, so it triggers 1. I've tried to draw it up (marking flow 1 as User #1 and flow 2 as User #2):

(What we try to do today is hit all four of these once — because we can't selectively listen for history or store changes, they will always trigger listen and subscribe respectively — and then stop the flow before we create a circle)

I think we inherently have two sources of truth for this lib: the URL and the router state in the redux store. Keeping these two in sync is what we're trying to solve.

[bman654]

I'm referring to your first use case. What is actually happening in #108 is the user is clicking on a ReactRouter.Link element, which then calls history.pushState(). But otherwise is consistent with your diagram.

The problem is that both Router and this library is calling history.listen. So Router sees the change and does a render. This library sees the change and updates the store and then updates history, etc.

This pull request adds some logic to detect the recursion and prevent step 7 from occurring, which would break the loop. But, from looking at the code, it seems like this isn't the only code related to breaking cycles.

I'm just proposing a different synchronization mechanism that has no cycles, so the information only flows in one direction.

In this version, this library would consume the history object and treat it as a write-only object to which it will write route changes (which ultimately get reflected in the browser url field). This library gives back a Mock History object that transforms mutation requests into Redux Actions, and subscribes to the Redux store to notify listeners.

Your scenario 2 works exactly the same as it currently works, except now there are no cycles to stop. Action updates store. Store notifies subscriber. One subscriber updates real History. Second subscriber updates MockHistory, which Router is listening to.

And now scenario 1 also no longer has any cycles. Router tries to modify history. Action is dispatched. Store is updated. Store subscriber updates real History. Store subscriber updates MockHistory, which Router is listening to.

I'll see if I can throw something together around this concept tonight.

[bman654]

I guess what my proposal really is: a ReduxHistory object that you can use with React-Router to make it redux-aware.

[kimjoar]

Not sure how this would work. You have both a MockHistory and a History? But User #1 click Router.Link is a URL update (and we either way need to handle a user randomly updating the URL directly). So you must have a history.listen somewhere. And that listen will also be triggered after step 4 in your last diagram (Store.subscribe -> History.pushState). So you still have a cycle in there, as far as I can see.

[bman654]

Router.Link just calls pushState on the history object that was given to the router and so never directly modifies the URL.

I don't understand what you mean by "user randomly updating the URL". You mean if the user literally goes into the URL bar and changes it? Doesn't that just cause the browser to load the new URL? Doesn't seem to trigger an event when I try it.

[kimjoar]

Try with createHashHistory, for example.

But either way, just to be sure I've understood, what you're proposing is for the user to not use history, but some other lib (e.g. a wrapper)? And this history lib does not update URLs, it dispatches actions instead.

[bman654]

Actually my idea is to use history library, but just as a history enhancer. But anyway I just realized what you meant. User could click browser back and forward buttons. My idea won't work with that without listening to the history object and then the cycles are back.

[kimjoar]

@jlongster

I don't think that's a systemic problem of avoidRouterUpdate

Agreed. The difficult thing is getting everything "to match up" — I always end up with a test or two that fails ;) Have been playing around a little bit with ideas without coming up with a simple solution.

[kimjoar]

Closing this in favour of #129