supasate / connected-react-router

A Redux binding for React Router v4
MIT License
4.73k stars 593 forks source link

Browser history not synched anymore with new router location starting from 6.6.0 #395

Open ziriax opened 4 years ago

ziriax commented 4 years ago

When reducing a state on some of our actions, we also output a new router location state using the following TS code:

interface RootState {
  router: RouterState;
  game: GameState;
}

function syncRouterLocation(state: RootState, pathname: string): RootState {
  const { router } = state;
  if (router.location.pathname !== pathname) {
    const location = createLocation(pathname);
    state = { ...state, router: { ...router, location } };
  }
  return state;
}

This code doesn't always work anymore with V6.6.0 and newer, the browser is sometimes not navigating to the new page (it depends on the starting page), I'm trying to find out what version broke it.

Our maybe our function above is just wrong, do we need to include other properties in the state? Do you have a helper function for this?

I guess it is this line that breaks our code, but just adding action: "PUSH" to the state doesn't seem to fix it.

What is the correct way to reduce the router location as a "side effect" in our own reducers?

agriffis commented 4 years ago

Time travelling is only intended for debugging with redux-devtools. Normally you should never write to the router's state from your app reducer. If this was working for you previously, I don't think it's how connected-react-router is intended to be used.

If you want to modify the location bar along with some of your own actions, you have a few choices:

What you have in the description is kind of like the last option, but you tried to do it in a reducer, which only worked by chance.

ziriax commented 4 years ago

Okay, I have worked around it, thanks for the info.

This does feel strange to me, the whole idea of Redux is that you have a state, and the view renders the state. I guess this paradigm breaks appart for browser history, since it is a stack, and can't just be "rendered".

ziriax commented 4 years ago

I'm reopening this issue, since I'm running circles with the proposed solutions, I guess I'm tackling this incorrectly.

The situation is as follows:

This works fine.

However, a SET_GAME action can also be dispatched differently, e.g. when the user loads a game by clicking a button. The reducer will update the game state, but now the Play page is rendered again, still with the old {name}, and it will load the old game again...

Setting the history location before SET_GAME is reduced won't work, since the Play page will then fetch the old game state.

Setting the history location after SET_GAME is reduced won't work, since the Play page will immediately render after the reduction took place with the wrong name...

So basically, I need to change the game state and the router state atomically, something that worked fine pre 6.6.0, even though it wasn't designed for this.

I could make different actions, but that doesn't seem to solve the problem.

I could pass the game state as the router location state, but that feels wrong, it should be independent...

This feels such a common scenario that a solution must exist for this?

For now I will revert to 6.5.2, since that solved all my problems ;-)

agriffis commented 4 years ago

However, a SET_GAME action can also be dispatched differently, e.g. when the user loads a game by clicking a button. The reducer will update the game state, but now the Play page is rendered again, still with the old {name}, and it will load the old game again...

Instead of SET_GAME can you push here?

ziriax commented 4 years ago

Instead of SET_GAME can you push here?

Yes, my example was simplified too much. SET_GAME is also used for game states that aren't saved on the server yet. I also want to be able to replay the redux logs, SET_GAME always carries a full state, nothing is ever loaded from the server when replaying actions. So basically, SET_GAME is the only action to provide a new state to the app.

I could solve my issue if I could detect if a page was rendered because a user navigated to it, because in all other cases, the program controls the state, and the page shouldn't alter it. Like the HTML input element, it only fires the change event when the user changed the element, not when code changed the value (as demonstrated here).

Maybe I could use the location state to detect the above? So when my middleware pushes the URL that corresponds to the next new game state that will be set, I pass a location state that says hey page, a new state is coming in, don't load one. Yes, that would actually work I guess? Amazing how just trying to explain a problem often results in finding a solution yourself, LOL ;-) The proof of the pudding is in the eating, I'm going to try this asap

Thanks for the feedback!

ziriax commented 4 years ago

Yes, that seems to work!

EDIT: No it doesn't, it seems a page refresh keeps the router location state somehow?

One small problem remains (independently from this): when replaying actions with the redux devtools, the initial page render doesn't realize it is being played back, and it will fetch the state from the server, dispatching a new action...

This is another issue: IMHO fetching state should rarely be done in a view, instead it should always be the result of a user or external event (e.g. the user clicking a button, the user navigating to a page).

But I can't find an event on the <Route/> for that, but that's a question for the React Router devs.

ziriax commented 4 years ago

It seems I was just using the wrong library for my architecture, https://github.com/faceyspacey/redux-first-router might suit me better?