-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: syncHistoryWithStore() #1362
Changes from 1 commit
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,12 +1,17 @@ | ||
import 'babel-polyfill' | ||
import React from 'react' | ||
import { render } from 'react-dom' | ||
import { browserHistory } from 'react-router' | ||
import Root from './containers/Root' | ||
import configureStore from './store/configureStore' | ||
import { syncHistoryWithStore } from './syncHistoryWithStore' | ||
|
||
const store = configureStore() | ||
const history = syncHistoryWithStore(browserHistory, store, { | ||
adjustUrlOnReplay: true | ||
}) | ||
|
||
render( | ||
<Root store={store} />, | ||
<Root store={store} history={history} />, | ||
document.getElementById('root') | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import * as ActionTypes from '../actions' | ||
import merge from 'lodash/merge' | ||
import paginate from './paginate' | ||
import { routeReducer } from 'react-router-redux' | ||
import { reducer as routing } from '../syncHistoryWithStore' | ||
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. why not just name the thing 'routing' in syncHistoryWithStore? I do this exact same thing in my code and my hunch is that other people do as well. 😄 |
||
import { combineReducers } from 'redux' | ||
|
||
// Updates an entity cache in response to any action with response.entities. | ||
|
@@ -50,8 +50,7 @@ const rootReducer = combineReducers({ | |
entities, | ||
pagination, | ||
errorMessage, | ||
routing: routeReducer | ||
routing | ||
}) | ||
|
||
|
||
export default rootReducer |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
export const WILL_NAVIGATE = '@@react-router/WILL_NAVIGATE' | ||
|
||
const initialState = { | ||
locationBeforeTransitions: null | ||
} | ||
|
||
// Mount this reducer to handle location changes | ||
export const reducer = (state = initialState, action) => { | ||
switch (action.type) { | ||
case WILL_NAVIGATE: | ||
// Use a descriptive name to make it less tempting to reach into it | ||
return { | ||
locationBeforeTransitions: action.locationBeforeTransitions | ||
} | ||
default: | ||
return state | ||
} | ||
} | ||
|
||
export function syncHistoryWithStore(history, store, { | ||
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. This is when I get to say “thank goodness histories are not classes, I told ya!” |
||
// Specify where you mounted the reducer | ||
selectLocationState = state => state.routing, | ||
adjustUrlOnReplay = false | ||
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. Since there are no action creators exposed and the state key is named to discourage access, it would seem the primary purpose is to sync up the URL bar after a replay. In that case, should this default to true? 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. Defaulting to true means that if you hydrate state from e.g. 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. You can detect that, because the initial location on load is a "POP" action. 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. How would I tell between the first POP action and a persisted session that happens to end on a POP? 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 confused. Why would it be surprising that restoring your serialized state, which syncs to history, would restore your URL bar? Wouldn't that be an expected behavior? |
||
} = {}) { | ||
let initialLocation | ||
let currentLocation | ||
let isTimeTraveling | ||
let unsubscribeFromStore | ||
let unsubscribeFromHistory | ||
|
||
// What does the store say about current location? | ||
const getLocationInStore = (useInitialIfEmpty) => { | ||
const locationState = selectLocationState(store.getState()) | ||
return locationState.locationBeforeTransitions || | ||
(useInitialIfEmpty ? initialLocation : undefined) | ||
} | ||
|
||
// Whenever store changes due to time travel, keep address bar in sync | ||
const handleStoreChange = () => { | ||
const locationInStore = getLocationInStore(true) | ||
if (currentLocation === locationInStore) { | ||
return | ||
} | ||
|
||
// Update address bar to reflect store state | ||
isTimeTraveling = true | ||
currentLocation = locationInStore | ||
history.replace(locationInStore) | ||
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. Comparable code in react-router-redux is https://github.com/rackt/react-router-redux/blob/b6a9c1bb51521bf6f5f416de394658d586683d9f/src/index.js#L80-L100 This is a little bit subtle – if you call 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. How do I reproduce the issue? I tried pressing Back and then toggling that action in DevTools, and it worked for me. 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. On react-router-redux? It looks like that was fixed with reactjs/react-router-redux#164 having been closed. More than that I don't know. What you will see, though, is that e.g. if you use hash history, the location key in the URL then won't match the location key in the Redux store. 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. The initial POP location can't be applied back to the browser location, so reseting the state in Dev Tools doesn't reset the URL bar. Replacing was a quick solution to achieve that. Perhaps 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. Why does that not break for toggling browser-level back actions? Those are 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. Let's discuss the current react-router-redux issues separately; it's just noise here. 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. @timdorr I think once you're toggling browser actions in DevTools with 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. @gaearon Just to finish up this discussion, can you try doing history.transitionTo({ ...locationInStore, action: 'PUSH' }) for this line? I believe it will be more correct and will avoid the odd edge cases I mentioned above. (alternatively, 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. This seems to preserve the key. I don’t see whether it impacts the behavior in any important way, but thanks for the tip. Fixed by 2e42dfa. 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 just avoids the edge cases I mentioned in #1362 (comment). There will still be a mismatch in |
||
isTimeTraveling = false | ||
} | ||
|
||
if (adjustUrlOnReplay) { | ||
unsubscribeFromStore = store.subscribe(handleStoreChange) | ||
handleStoreChange() | ||
} | ||
|
||
// Whenever location changes, dispatch an action to get it in the store | ||
const handleLocationChange = (location) => { | ||
// ... unless we just caused that location change | ||
if (isTimeTraveling) { | ||
return | ||
} | ||
|
||
// Remember where we are | ||
currentLocation = location | ||
|
||
// Are we being called for the first time? | ||
if (!initialLocation) { | ||
// Remember as a fallback in case state is reset | ||
initialLocation = location | ||
|
||
// Respect persisted location, if any | ||
if (getLocationInStore()) { | ||
return | ||
} | ||
} | ||
|
||
// Tell the store to update by dispatching an action | ||
store.dispatch({ | ||
type: WILL_NAVIGATE, | ||
locationBeforeTransitions: location | ||
}) | ||
} | ||
unsubscribeFromHistory = history.listen(handleLocationChange) | ||
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. Normally history enhancers don't call 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, we're purposefully eavesdropping here. Can make this lazy if necessary although then we have a risk of missing initial actions (I think). |
||
|
||
// The enhanced history uses store as source of truth | ||
return Object.assign({}, history, { | ||
// The listeners are subscribed to the store instead of history | ||
listen(listener) { | ||
return store.subscribe(() => | ||
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 what the guarantees with 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. Ah, spot on! 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. Should be fixed in e271f55. |
||
listener(getLocationInStore(true)) | ||
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. TODO (optimization): check if location has changed and don't call listener if it hasn't. |
||
) | ||
}, | ||
|
||
// It also provides a way to destroy internal listeners | ||
dispose() { | ||
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. TODO (design): can we agree on a standard way of disposing resources required by histories, if any? Then we could call underlying 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. The standard way is that we have this cruft in like a half dozen different places by now: https://github.com/rackt/history/blob/v2.0.0-rc3/modules/createBrowserHistory.js#L104-L132. This would be easier if histories were classes, but they're not 😛 |
||
if (adjustUrlOnReplay) { | ||
unsubscribeFromStore() | ||
} | ||
unsubscribeFromHistory() | ||
} | ||
}) | ||
} |
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.
Note: if you pass
false
DevTools history time travel will still work in UI, it just won't update the address bar.