-
Notifications
You must be signed in to change notification settings - Fork 640
Use middleware to synchronize store to history #141
Changes from all commits
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 |
---|---|---|
@@ -1,4 +1,3 @@ | ||
{ | ||
"presets": ["es2015"], | ||
"plugins": ["transform-object-assign"] | ||
"presets": ["es2015", "stage-2"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,149 +1,112 @@ | ||
import deepEqual from 'deep-equal' | ||
|
||
// Constants | ||
|
||
export const UPDATE_PATH = '@@router/UPDATE_PATH' | ||
export const TRANSITION = '@@router/TRANSITION' | ||
export const UPDATE_LOCATION = '@@router/UPDATE_LOCATION' | ||
|
||
const SELECT_STATE = state => state.routing | ||
|
||
export function pushPath(path, state, { avoidRouterUpdate = false } = {}) { | ||
return { | ||
type: UPDATE_PATH, | ||
payload: { | ||
path: path, | ||
state: state, | ||
replace: false, | ||
avoidRouterUpdate: !!avoidRouterUpdate | ||
} | ||
} | ||
function transition(method) { | ||
return arg => ({ | ||
type: TRANSITION, | ||
method, arg | ||
}) | ||
} | ||
|
||
export function replacePath(path, state, { avoidRouterUpdate = false } = {}) { | ||
export const push = transition('push') | ||
export const replace = transition('replace') | ||
|
||
// TODO: Add go, goBack, goForward. | ||
|
||
function updateLocation(location) { | ||
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 assuming this 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 know that makes the API slightly asymmetrical (the state would be different than what you passed in), but now that we can always store the same shape of object in the state I think it's ok. 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. It's already supported. See e.g. https://github.com/rackt/redux-simple-router/pull/141/files#diff-413f2628e9c93c46cec6db202ddb8ea4R338. 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. Oops - misread. I'm actually not exporting 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, great, looks like that's a natively-supported thing in react-router. 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. Yes – from the |
||
return { | ||
type: UPDATE_PATH, | ||
payload: { | ||
path: path, | ||
state: state, | ||
replace: true, | ||
avoidRouterUpdate: !!avoidRouterUpdate | ||
} | ||
type: UPDATE_LOCATION, | ||
location | ||
} | ||
} | ||
|
||
// Reducer | ||
|
||
let initialState = { | ||
changeId: 1, | ||
path: undefined, | ||
state: undefined, | ||
replace: false | ||
const initialState = { | ||
location: undefined | ||
} | ||
|
||
function update(state=initialState, { type, payload }) { | ||
if(type === UPDATE_PATH) { | ||
return Object.assign({}, state, { | ||
path: payload.path, | ||
changeId: state.changeId + (payload.avoidRouterUpdate ? 0 : 1), | ||
state: payload.state, | ||
replace: payload.replace | ||
}) | ||
export function routeReducer(state = initialState, { type, location }) { | ||
if (type !== UPDATE_LOCATION) { | ||
return state | ||
} | ||
return state | ||
|
||
return { location } | ||
} | ||
|
||
// Syncing | ||
|
||
function locationsAreEqual(a, b) { | ||
return a != null && b != null && a.path === b.path && deepEqual(a.state, b.state) | ||
} | ||
export function syncHistory(history) { | ||
let unsubscribeHistory, currentKey, unsubscribeStore | ||
let connected = false, syncing = false | ||
|
||
function createPath(location) { | ||
const { pathname, search, hash } = location | ||
let result = pathname | ||
if (search) | ||
result += search | ||
if (hash) | ||
result += hash | ||
return result | ||
} | ||
function middleware(store) { | ||
unsubscribeHistory = history.listen(location => { | ||
currentKey = location.key | ||
if (syncing) { | ||
// Don't dispatch a new action if we're replaying location. | ||
return | ||
} | ||
|
||
export function syncReduxAndRouter(history, store, selectRouterState = SELECT_STATE) { | ||
const getRouterState = () => selectRouterState(store.getState()) | ||
|
||
// To properly handle store updates we need to track the last route. | ||
// This route contains a `changeId` which is updated on every | ||
// `pushPath` and `replacePath`. If this id changes we always | ||
// trigger a history update. However, if the id does not change, we | ||
// check if the location has changed, and if it is we trigger a | ||
// history update. It's possible for this to happen when something | ||
// reloads the entire app state such as redux devtools. | ||
let lastRoute = undefined | ||
|
||
if(!getRouterState()) { | ||
throw new Error( | ||
'Cannot sync router: route state does not exist (`state.routing` by default). ' + | ||
'Did you install the routing reducer?' | ||
) | ||
} | ||
store.dispatch(updateLocation(location)) | ||
}) | ||
|
||
const unsubscribeHistory = history.listen(location => { | ||
const route = { | ||
path: createPath(location), | ||
state: location.state | ||
} | ||
connected = true | ||
|
||
if (!lastRoute) { | ||
// `initialState` *should* represent the current location when | ||
// the app loads, but we cannot get the current location when it | ||
// is defined. What happens is `history.listen` is called | ||
// immediately when it is registered, and it updates the app | ||
// state with an UPDATE_PATH action. This causes problem when | ||
// users are listening to UPDATE_PATH actions just for | ||
// *changes*, and with redux devtools because "revert" will use | ||
// `initialState` and it won't revert to the original URL. | ||
// Instead, we specialize the first route notification and do | ||
// different things based on it. | ||
initialState = { | ||
changeId: 1, | ||
path: route.path, | ||
state: route.state, | ||
replace: false | ||
return next => action => { | ||
if (action.type !== TRANSITION || !connected) { | ||
next(action) | ||
return | ||
} | ||
|
||
// Also set `lastRoute` so that the store subscriber doesn't | ||
// trigger an unnecessary `pushState` on load | ||
lastRoute = initialState | ||
// FIXME: Is it correct to swallow the TRANSITION action here and replace | ||
// it with UPDATE_LOCATION instead? We could also use the same type in | ||
// both places instead and just set the location on the action. | ||
|
||
store.dispatch(pushPath(route.path, route.state, { avoidRouterUpdate: true })); | ||
} else if(!locationsAreEqual(getRouterState(), route)) { | ||
// The above check avoids dispatching an action if the store is | ||
// already up-to-date | ||
const method = location.action === 'REPLACE' ? replacePath : pushPath | ||
store.dispatch(method(route.path, route.state, { avoidRouterUpdate: true })) | ||
const { method, arg } = action | ||
history[method](arg) | ||
} | ||
}) | ||
|
||
const unsubscribeStore = store.subscribe(() => { | ||
let routing = getRouterState() | ||
|
||
// Only trigger history update if this is a new change or the | ||
// location has changed. | ||
if(lastRoute.changeId !== routing.changeId || | ||
!locationsAreEqual(lastRoute, routing)) { | ||
} | ||
|
||
lastRoute = routing | ||
const method = routing.replace ? 'replace' : 'push' | ||
history[method]({ | ||
pathname: routing.path, | ||
state: routing.state | ||
middleware.syncHistoryToStore = | ||
(store, selectRouterState = SELECT_STATE) => { | ||
const getRouterState = () => selectRouterState(store.getState()) | ||
const { location: initialLocation } = getRouterState() | ||
|
||
unsubscribeStore = store.subscribe(() => { | ||
const { location } = getRouterState() | ||
|
||
// If we're resetting to the beginning, use the saved initial value. We | ||
// need to dispatch a new action at this point to populate the store | ||
// appropriately. | ||
if (!location) { | ||
history.transitionTo(initialLocation) | ||
return | ||
} | ||
|
||
// Otherwise, if we need to update the history location, do so without | ||
// dispatching a new action, as we're just bringing history in sync | ||
// with the store. | ||
if (location.key !== currentKey) { | ||
syncing = true | ||
history.transitionTo(location) | ||
syncing = false | ||
} | ||
}) | ||
} | ||
|
||
}) | ||
|
||
return function unsubscribe() { | ||
middleware.unsubscribe = () => { | ||
unsubscribeHistory() | ||
unsubscribeStore() | ||
if (unsubscribeStore) { | ||
unsubscribeStore() | ||
} | ||
|
||
connected = false | ||
} | ||
} | ||
|
||
export { update as routeReducer } | ||
return middleware | ||
} |
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.
push
andreplace
are very vague names when seen outside of react-router. Would you be ok keeping the Path suffix? I think it generally makes it clearer.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.
I agree that
push
andreplace
are awful names. MaybepushLocation
andreplaceLocation
? I thoughtpushPath
was a bit inaccurate, since this would let you pushquery
, &c. as well. I also didn't want to figure out how to then namego
,goBack
,goForward
– would we suffix those withlocation
as well?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.
Hm that's a good point.
goPath
doesn't really sound great. We could just renamepush
andreplace
and leave the other be, but if you think that's confusing we can just stick with these simple names and users can rename them when importing.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.
I don't know the naming conventions for Redux action creators at all. Is there an option to namescope these under e.g.
HistoryActionCreators
or something, like it's done with the devtools action creators?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.
This could work:
It would also make using
bindActionCreators
easier because they're all under the same object namespace.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.
Case in point:
I like that a lot.
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.
PR it then :p
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.
Done! #149