Make react-router-redux work with time travelling

[supasate]

Hi @timdorr,

This is my first attempt to make it work with time travelling.

• I use a similar approach with connected-react-router.
• The test cases are from old react-router-redux.

I make a repo to demonstrate that it starts working now.

However, there are two known problems.

1. Internal browser stack is modified when time travelling. It should be later fixed with Hook an external listener zalmoxisus/redux-devtools-instrument#15, so, I ignore it for now.

2. reset test case is fail. This is crucial and needs to discuss.

This problem is caused by the reset action will reset the store to its initial state which its location state is null and cannot be pushed to the history object.

In connected-react-router, we solved this issue by providing the initial history to be the initial location state in the store like the following:

const store = createStore(
  connectRouter(history)(rootReducer), // new root reducer with router state
  initialState,
  compose(
    ...
  ),
)

This pattern (higher order reducer) has some advantages.

1. The location in the store will begin with the initial history location instead of null (and the devtools's reset() will bring the store to match with the first URL request).
2. It doesn't trigger LOCATION_CHANGE action at the beginning.
3. Users don't need to manually mount routerReducer to the root reducer. It will be done automatically with the key router.

I also make a PR to connected-react-router to check that it works with same test case.

So, if you think this pattern is the right way to go, I will try refactoring the reducer.

[timdorr]

Hmm, will this trigger the same problems as #4713?

[supasate]

Right. So, my idea is going to refactor it to listen the history in the constructor and dispatch it in the listener instead (actually, the same approach I did here).

[mjackson]

Internal browser stack is modified when time traveling. It should be later fixed with zalmoxisus/redux-devtools-instrument#15, so, I ignore it for now.

I actually don't think that issue is going to change anything.

[supasate]

@mjackson I think if we know the history index which provided by a listener, we can use history.go to jump back and forth instead of history.push.

[mjackson]

@supasate Yep, that was my original theory. But I still don't think that issue is going to change anything.

[supasate]

@mjackson Can we leave that issue to solve later when there is appropriate APIs or alternative ideas?

I'd also like to get feedback on the following issue.

2. reset test case is fail. This is crucial and needs to discuss.

This problem is caused by the reset action will reset the store to its initial state which its location state is null and cannot be pushed to the history object.
In connected-react-router, we solved this issue by providing the initial history to be the initial location state in the store like the following:

const store = createStore(
  connectRouter(history)(rootReducer), // new root reducer with router state
  initialState,
  compose(
    ...
  ),
)

[zalmoxisus]

In the current PR, this.inTimeTravelling will be true not only for time travelling (moving back and forth), but also for hot-reloading (replacing root reducer), importing state, cancelling (skipping) actions and reordering actions. It's not an issue here as we want to update history's location for those cases as well.

It would be an issue when we want to address supasate/connected-react-router#36. I don't see other solution for that than described in zalmoxisus/redux-devtools-instrument#15 (comment). Relying on liftedStore state wouldn't be a problem since we want to handle devtools only when it's in use (its enhancer is applied). The idea in zalmoxisus/redux-devtools-instrument#15 would make the usage too complex, which isn't the case of zalmoxisus/redux-devtools-instrument#16.

[timdorr]

@supasate I just made some big changes on the ConnectedRouter. It now relies on a history listener, rather than "subscribing" via <Route>. That also flattens the component tree a bit.

This will need a bit of a refactor then. My general plan is to copy most of what we did with 4.0.8, so we can handle rewinds and let users decide if the URL bar should update or not.

[supasate]

@timdorr I rebased the code and now made it pass the Devtools test cases (by adapting your 4.0.8 about firing initial location again if the store is reset). I don't understand all your code yet, so, I tried to simplify it and might miss some cases, please advise if you find anything went wrong.

[supasate]

@timdorr is there any update on this issue?

[timdorr]

Nope, I'm busy on a side project that has a deadline of next Friday (worst case scenario). I hope to have some time for OSS coding soon!

[supasate]

Understandable! No rush. Take your time!

[zarv1k]

Is there any updates on this amazing feature?

[nihgwu]

ping @timdorr

[timdorr]

Pong!

Sorry, I've been turbo busy lately with a new product release. I'm going to be switching jobs next month, and have some time taken off between them where I'll devote some time to this and other projects.

[mjackson]

This is going to break the browser's history. You cannot just push the location onto the history stack because this will create a new entry in the history. This means you're always going forward in the history, even if the intent was to go back.

What you really need to do here is history.go(n) where n is the difference in the index of the current location in the history and the index of the storeLocation, or the location you want to get to. But as much as I've tried to think about it over the years, I can't figure out a reliable way to get that n.

[pshrmn]

@mjackson In Hickory, instead of using random keys I use incremental values of major.minor where major increments when pushing and minor increments when replacing. Then, determining n is just a matter of parsing the keys for the two locations and calculating the difference between majors.

Perhaps I overlooked something when implementing that, but I haven't thought of any reason that that shouldn't work.

I should note that I took the easy route and always use window.history so that I can rely on state, even with hash history. Decided to just support IE10+ and newish Android. http://caniuse.com/#feat=history

[mjackson]

Yes, if we could always rely on state then we could do it for sure. You're right, we should just do that. I need to stop thinking about supporting old browsers.

[guywaldman]

Hey guys,

Any progress on this?

P.S. Love your work! Thank you so much 😄

[PeteOnTheGitHub]

This would be a fantastic feature

[Armour]

Hi, any progress on this PR?

I temporarily fixed the bug on my boilerplate using @supasate 's connected-react-router, (if you want to see the working example) and I'm glad to see its gonna merge with the official react-router-redux, once this PR is done I'll be happy to switch back to use the react-router-redux again.

Thanks so much for you all build this awesome tool 😆

[8enSmith]

Like others here, I too would love to see react-router-redux work with time travelling!

[GioLogist]

This would be pretty awesome!

[timdorr]

I would too, but I've had zero motivation to work on this because it's always going to be quirky and lead to a lot more pain than actually helping DX. If someone wants to build on this PR, feel free to submit a new one that uses this code as a basis (or carve your own path).

[jjmschofield]

It would be amazing to have this. Is it tracked by an issue? I couldn't see one :(

[mgcrea]

Just adding my 2 cents as I was hitting this + another bug while I was trying to quickly replay a stored state exported from devtools at startup (did properly connect the location prop to prevent update blocking but it would not work):

import state from './state.json';
class App extends PureComponent {
  componentDidMount() {
    if (state.payload) {
      const actions = JSON.parse(state.payload);
      actions.forEach(action => store.dispatch(action));
    }
  }
  render() {
    return (
      <Provider store={store}>
        <AppRouter />
      </Provider>
    );
  }
}

I can confirm that using connected-react-router did both fix the time-traveling issue and my restore state issue.

As a side note, @timdorr I know you must be really busy but your feedback sounds a bit foggy for me and not really actionable, I did not really understand what issues you had with this PR and what would make you accept a new PR on this.

Anyway, thanks for the hard work!

[timdorr]

Closing this out since react-router-redux is deprecated. Use connected-react-router instead!