Skip to content
This repository was archived by the owner on Oct 26, 2018. It is now read-only.

Prevent infinite loops when state is updated before the actual history change is fired #30

Merged
merged 1 commit into from
Nov 22, 2015
Merged

Prevent infinite loops when state is updated before the actual history change is fired #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ function locationToString(location) {
}

function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
let lastRoute;
const getRouterState = () => selectRouterState(store.getState());

if(!getRouterState()) {
Expand All @@ -47,22 +48,26 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) {
}

const unsubscribeHistory = history.listen(location => {
const newLocation = locationToString(location);
// Avoid dispatching an action if the store is already up-to-date,
// even if `history` wouldn't do anything if the location is the same
if(getRouterState().path !== locationToString(location)) {
store.dispatch(updatePath(locationToString(location)));
if(getRouterState().path !== newLocation) {
lastRoute = newLocation;
store.dispatch(updatePath(newLocation));
}
});

const unsubscribeStore = store.subscribe(() => {
const routing = getRouterState();

// Don't update the router if nothing has changed. The
// `noRouterUpdate` flag can be set to avoid updating altogether,
// Don't update the router if the routing state hasn't changed or the new routing path
// is already the current location.
// The `noRouterUpdate` flag can be set to avoid updating altogether,
// which is useful for things like loading snapshots or very special
// edge cases.
if(routing.path !== locationToString(window.location) &&
if(lastRoute !== routing.path && routing.path !== locationToString(window.location) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about why this is necessary? This approach generally looks fine to me (although I still don't understand why you need to pass true in the updatePath action above)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I'll add a comment. Please let me know if my assumption about the true in updatePath is correct

!routing.noRouterUpdate) {
lastRoute = routing.path;
history.pushState(null, routing.path);
}
});
Expand All @@ -77,5 +82,5 @@ module.exports = {
UPDATE_PATH,
updatePath,
syncReduxAndRouter,
routeReducer: update,
routeReducer: update
};