-
Notifications
You must be signed in to change notification settings - Fork 640
Handle routing state and replace #38
Changes from all commits
5e41803
b7f10aa
a0d6945
be81be6
3581f20
2a65890
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,5 +36,8 @@ | |
"isparta": "^4.0.0", | ||
"mocha": "^2.3.4", | ||
"redux": "^3.0.4" | ||
}, | ||
"dependencies": { | ||
"deep-equal": "^1.0.1" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,42 +1,59 @@ | ||
const deepEqual = require('deep-equal'); | ||
|
||
// constants | ||
// Constants | ||
|
||
const UPDATE_PATH = "@@router/UPDATE_PATH"; | ||
const SELECT_STATE = state => state.routing; | ||
|
||
// Action creator | ||
|
||
function updatePath(path, avoidRouterUpdate) { | ||
function pushPath(path, state, opts) { | ||
opts = opts || {}; | ||
return { | ||
type: UPDATE_PATH, | ||
path: path, | ||
avoidRouterUpdate: !!avoidRouterUpdate | ||
state: state, | ||
replace: false, | ||
avoidRouterUpdate: !!opts.avoidRouterUpdate | ||
}; | ||
} | ||
|
||
function replacePath(path, state, opts) { | ||
opts = opts || {}; | ||
return { | ||
type: UPDATE_PATH, | ||
path: path, | ||
state: state, | ||
replace: true, | ||
avoidRouterUpdate: !!opts.avoidRouterUpdate | ||
} | ||
} | ||
|
||
// Reducer | ||
|
||
const initialState = { | ||
changeId: 1, | ||
path: (typeof window !== 'undefined') ? | ||
locationToString(window.location) : | ||
'/' | ||
path: undefined, | ||
state: undefined, | ||
replace: false | ||
}; | ||
|
||
function update(state=initialState, action) { | ||
if(action.type === UPDATE_PATH) { | ||
return Object.assign({}, state, { | ||
path: action.path, | ||
changeId: state.changeId + (action.avoidRouterUpdate ? 0 : 1) | ||
changeId: state.changeId + (action.avoidRouterUpdate ? 0 : 1), | ||
state: action.state, | ||
replace: action.replace | ||
}); | ||
} | ||
return state; | ||
} | ||
|
||
// Syncing | ||
|
||
function locationToString(location) { | ||
return location.pathname + location.search + location.hash; | ||
function locationsAreEqual(a, b) { | ||
return a.path === b.path && deepEqual(a.state, b.state); | ||
} | ||
|
||
function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { | ||
|
@@ -51,24 +68,31 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { | |
} | ||
|
||
const unsubscribeHistory = history.listen(location => { | ||
const routePath = locationToString(location); | ||
const route = { | ||
path: history.createPath(location), | ||
state: location.state | ||
}; | ||
|
||
// 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(locationsAreEqual(getRouterState(), route)) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we actually even need this check anymore. The only reason is existed was to stop the cyclical update between redux and the router. But now that cycle is broken with the I think we can just remove it, and always fire a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good idea. I got some failing tests when just removing it, so I'll try to take a look at it and find a solution There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, interesting. Would love to know why. It's so fun to reduce it to as little code as possible. I'm cool with deep-equal if that's really what we need to do though. |
||
|
||
// Avoid dispatching an action if the store is already up-to-date | ||
if(getRouterState().path !== routePath) { | ||
store.dispatch(updatePath(routePath, { avoidRouterUpdate: true })); | ||
} | ||
store.dispatch(pushPath(route.path, route.state, { avoidRouterUpdate: true })); | ||
}); | ||
|
||
const unsubscribeStore = store.subscribe(() => { | ||
const routing = getRouterState(); | ||
|
||
// Only update the router once per `updatePath` call. This is | ||
// Only update the router once per `pushPath` call. This is | ||
// indicated by the `changeId` state; when that number changes, we | ||
// should call `pushState`. | ||
if(lastChangeId !== routing.changeId) { | ||
lastChangeId = routing.changeId; | ||
history.pushState(null, routing.path); | ||
} | ||
// should update the history. | ||
if(lastChangeId === routing.changeId) return; | ||
|
||
lastChangeId = routing.changeId; | ||
|
||
const method = routing.replace ? 'replaceState' : 'pushState'; | ||
|
||
history[method](routing.state, routing.path); | ||
}); | ||
|
||
return function unsubscribe() { | ||
|
@@ -79,7 +103,8 @@ function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { | |
|
||
module.exports = { | ||
UPDATE_PATH, | ||
updatePath, | ||
pushPath, | ||
replacePath, | ||
syncReduxAndRouter, | ||
routeReducer: update | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure we need to
deepEqual
here? I'm having a hard time wrapping my head around this, but probably because I haven't use that state much.Hm, I have a thought... read my 2nd comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically a copy of https://github.com/rackt/history/blob/master/modules/createHistory.js#L12-L18