Stopping cycles

[kimjoar]

There are two flows to manipulate the browser history:

1. Update URL/history
   -> history callback
   -> dispatch push/replace action
   -> store callback

2. Dispatch push/replace action
   -> store callback
   -> update history
   -> history callback

In both cases we call both of our callbacks, and in both cases we need to make sure that the flow stops before we create a cycle. E.g. let's say we want to change the URL by dispatching an action. This action triggers a history update, and when handling this update we have to make sure we don't dispatch yet another time.

To solve both cases without relying on deep-equal I think we need to track two variables — one to stop each flow.

To handle case 1 we set { avoidRouterUpdate: true } in the history callback. As we don't increment the changeId we can stop the flow in the store callback by comparing it against the lastChangeId. By using changeId we also enable actions in history.listenBefore callbacks.

To handle case 2 we need to stop after we have updated the history based on a pushState or replaceState action. The problem is how to keep track of this update. In case 1 we decide to not increment the change id, thereby stopping the flow, but when updating the history I'm not sure how we can do the same. Right now we rely on comparing the current router state location with the new history location, but it would be nice to not need the deep-equal dependency in #38.

One option is to keep some kind of state in location.state in the same way as we have changeId in the store state, but I'm not sure if that would work (We don't have the same control over the history methods as we have over the pushPath and replacePath actions). Another option is to add another variable, e.g. isRoutingFromAction, which keeps track of whether or not the current route change was triggered by an action. Basically something like this:

let isRoutingFromAction = false;

const unsubscribeHistory = history.listen(location => {
  if (isRoutingFromAction) {
    // By handling this we have stopped the cycle, and
    // therefore need to reset the state.
    isRoutingFromAction = false;
    return;
  }

  // ...
});

const unsubscribeStore = store.subscribe(() => {
  const routing = getRouterState();
  if(lastChangeId === routing.changeId) return;

  isRoutingFromAction = true;

  // ...
});

With this change all tests still pass and my app still works, but as we don't run the tests in browsers I'm not entirely sure about potential edge cases (e.g. I don't use listenBefore at all in my app).

(I can create a PR with this if anyone wants to play around with it)

Maybe someone has other ideas about how to handle this?

[kimjoar]

I tested my isRoutingFromAction solution using the browser test setup in #52 and everything is green. So the question is of course if the tests cover all the edge cases — so we at least need to add some tests using the other history implementations. I'll try to get a PR in, but I still think #38 should be merged as is while we keep working on the right solution

[kimjoar]

Testing isRoutingFromAction with #52 and createHashHistory yields several errors, e.g. this related to history.listenBefore:

Uncaught Error: Expected { changeId: 6, path: '/bar', replace: false, state: undefined } to equal { changeId: 2, path: '/foo', replace: false, state: undefined }

Everything is green with createMemoryHistory.

[jlongster]

@kjbekkelund Thanks a bunch for looking into this. Previously I was thinking there might be a simple way to get around it; but the methods I was thinking of would generate extra actions which are harmless but would clutter the devtools and be confusing to people tracking actions.

Most people probably don't use this state, and it will just be null. I'm fine with the deep-equal now, if you think that's the way to go. The above technique could work but it also seems like it makes the code a little more confusing. What do you think?

[kimjoar]

@jlongster I think we should keep deep-equal for now (while we keep thinking of better and cleaner ways to solve this). I'll just close this for now.

[jlongster]

Sounds good to me. I appreciate your research. Most of the time state will be null anyway.