reactjs / react-router-redux

Ruthlessly simple bindings to keep react-router and redux in sync
https://www.npmjs.com/package/react-router-redux
MIT License
7.82k stars 647 forks source link

syncHistoryWithStore does not respect redux state #534

Open pronebird opened 7 years ago

pronebird commented 7 years ago

Hi,

I am trying to restore the displayed page on app startup but it's always /, redux store is preloaded from local storage. HoweversyncHistoryWithStore erases the state.

I use redux-localstorage to persist redux to local storage which seems to be merging state from local storage into initialState therefore restoring the state without telling anyone which shouldn't be any different for the app when the concept of single source of truth is applied.

I am sure this issue can be reproduced with initialState with hardcoded route.

import { routerMiddleware, routerReducer as routing } from 'react-router-redux';
import persistState from 'redux-localstorage';
import thunk from 'redux-thunk';
import routes from './routes';

const router = routerMiddleware(hashHistory);

const reducers = {
  routing
};

const middlewares = [ thunk, router ];

function configureStore(initialState) {
  const enhancer = compose(applyMiddleware(...middlewares), persistState());
  const rootReducer = combineReducers(reducers);

  return createStore(rootReducer, initialState, enhancer);
}

const initialState = {};
const store = configureStore(initialState);
const routerHistory = syncHistoryWithStore(hashHistory, store);

ReactDOM.render(
  <Provider store={ store }>
    <Router history={ routerHistory } routes={ routes } />
  </Provider>,
  rootElement
);

I tried adjustUrlOnReplay without luck:

const routerHistory = syncHistoryWithStore(hashHistory, store, { adjustUrlOnReplay: true });

Thanks

pronebird commented 7 years ago

Debugging through syncHistoryWithStore reveals that the following statement breaks:

if (currentLocation === locationInStore || initialLocation === locationInStore) {
        return;
}

in my case initialLocation restored from state and is /connect and is the same as locationInStore, therefore syncHistoryWithStore does not attempt to restore the location. That seems to be error prone in its essence:

// Init initialLocation with potential location in store
initialLocation = getLocationInStore();

initialLocation === locationInStore will always be true on cold restart with state restored from local storage. This code will only work if we push new location first.

Why not to consult history for initial location? I mean, browser should know better right? When the app cold starts / is initial location of browser, but state has /connect set.

pronebird commented 7 years ago

As a workaround we could simply enforce the transition to new route right after syncHistoryWithStore. Since syncHistoryWithStore will always reset the route to /, we could save the last visited location prior to the call and then feed it back, i.e:

const recentLocation = (store.getState().routing || {}).locationBeforeTransitions;
const routerHistory = syncHistoryWithStore(hashHistory, store, { adjustUrlOnReplay: true });
if(recentLocation && recentLocation.pathname) {
  routerHistory.replace(recentLocation.pathname);
}
rob2d commented 7 years ago

Just a random note, it seems the syncHistoryWithStore method isn't a necessity in v5.0.0 as the middleware takes care of it. So perhaps if migrating to the alpha for that, no more issues? [outside of... the alpha thing]

MacKentoch commented 7 years ago

It seems like react-router-redux v5.0.0 will fix a lot of things when using react-router 4. But what about using immutable state, will a custom routerReducer do the job:

import { fromJS }           from 'immutable';
import { LOCATION_CHANGE }  from 'react-router-redux';

const initialState = fromJS({
  locationBeforeTransitions: null
});

export default (state = initialState, action) => {
  if (action.type === LOCATION_CHANGE) {
    return state.set('locationBeforeTransitions', action.payload);
  }

  return state;
};

Or middleware will take care of it?

rowlandekemezie commented 6 years ago

@MacKentoch have you gotten a workaround for your question ☝️

MacKentoch commented 6 years ago

@mentrie I ended my starter with react-router-redux 4.x.x and wrote my own reducer.

I did not give a try to 5.x since I did have the use case in private or pro project then.

But, I keep interesting in new about it