Skip to content
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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
12 changes: 5 additions & 7 deletions examples/real-world/containers/App.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { Component, PropTypes } from 'react'
import { connect } from 'react-redux'
import { push } from 'react-router-redux'
import { browserHistory } from 'react-router'
import Explore from '../components/Explore'
import { resetErrorMessage } from '../actions'

Expand All @@ -17,7 +17,7 @@ class App extends Component {
}

handleChange(nextValue) {
this.props.push(`/${nextValue}`)
browserHistory.push(`/${nextValue}`)
}

renderErrorMessage() {
Expand Down Expand Up @@ -56,20 +56,18 @@ App.propTypes = {
// Injected by React Redux
errorMessage: PropTypes.string,
resetErrorMessage: PropTypes.func.isRequired,
push: PropTypes.func.isRequired,
inputValue: PropTypes.string.isRequired,
// Injected by React Router
children: PropTypes.node
}

function mapStateToProps(state) {
function mapStateToProps(state, ownProps) {
return {
errorMessage: state.errorMessage,
inputValue: state.routing.location.pathname.substring(1)
inputValue: ownProps.location.pathname.substring(1)
}
}

export default connect(mapStateToProps, {
resetErrorMessage,
push
resetErrorMessage
})(App)
4 changes: 2 additions & 2 deletions examples/real-world/containers/RepoPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ RepoPage.propTypes = {
loadStargazers: PropTypes.func.isRequired
}

function mapStateToProps(state, props) {
const { login, name } = props.params
function mapStateToProps(state, ownProps) {
const { login, name } = ownProps.params
const {
pagination: { stargazersByRepo },
entities: { users, repos }
Expand Down
6 changes: 3 additions & 3 deletions examples/real-world/containers/Root.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@ import React, { Component, PropTypes } from 'react'
import { Provider } from 'react-redux'
import routes from '../routes'
import DevTools from './DevTools'
import { Router, browserHistory } from 'react-router'
import { Router } from 'react-router'

export default class Root extends Component {
render() {
const { store } = this.props
const { store, history } = this.props
return (
<Provider store={store}>
<div>
<Router history={browserHistory} routes={routes} />
<Router history={history} routes={routes} />
<DevTools />
</div>
</Provider>
Expand Down
6 changes: 3 additions & 3 deletions examples/real-world/containers/Root.prod.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import React, { Component, PropTypes } from 'react'
import { Provider } from 'react-redux'
import routes from '../routes'
import { Router, browserHistory } from 'react-router'
import { Router } from 'react-router'

export default class Root extends Component {
render() {
const { store } = this.props
const { store, history } = this.props
return (
<Provider store={store}>
<Router history={browserHistory} routes={routes} />
<Router history={history} routes={routes} />
</Provider>
)
}
Expand Down
4 changes: 2 additions & 2 deletions examples/real-world/containers/UserPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ UserPage.propTypes = {
loadStarred: PropTypes.func.isRequired
}

function mapStateToProps(state, props) {
const { login } = props.params
function mapStateToProps(state, ownProps) {
const { login } = ownProps.params
const {
pagination: { starredByUser },
entities: { users, repos }
Expand Down
7 changes: 6 additions & 1 deletion examples/real-world/index.js
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
Copy link
Contributor Author

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.

})

render(
<Root store={store} />,
<Root store={store} history={history} />,
document.getElementById('root')
)
1 change: 0 additions & 1 deletion examples/real-world/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
"react-dom": "^0.14.7",
"react-redux": "^4.2.1",
"react-router": "2.0.0-rc5",
"react-router-redux": "^2.1.0",
"redux": "^3.2.1",
"redux-logger": "^2.4.0",
"redux-thunk": "^1.0.3"
Expand Down
5 changes: 2 additions & 3 deletions examples/real-world/reducers/index.js
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'
Copy link

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -50,8 +50,7 @@ const rootReducer = combineReducers({
entities,
pagination,
errorMessage,
routing: routeReducer
routing
})


export default rootReducer
13 changes: 3 additions & 10 deletions examples/real-world/store/configureStore.dev.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,20 @@
import { createStore, applyMiddleware, compose } from 'redux'
import { syncHistory } from 'react-router-redux'
import { browserHistory } from 'react-router'
import DevTools from '../containers/DevTools'
import thunk from 'redux-thunk'
import api from '../middleware/api'
import createLogger from 'redux-logger'
import api from '../middleware/api'
import rootReducer from '../reducers'

const reduxRouterMiddleware = syncHistory(browserHistory)
import DevTools from '../containers/DevTools'

export default function configureStore(initialState) {
const store = createStore(
rootReducer,
initialState,
compose(
applyMiddleware(thunk, api, reduxRouterMiddleware, createLogger()),
applyMiddleware(thunk, api, createLogger()),
DevTools.instrument()
)
)

// Required for replaying actions from devtools to work
reduxRouterMiddleware.listenForReplays(store)

if (module.hot) {
// Enable Webpack hot module replacement for reducers
module.hot.accept('../reducers', () => {
Expand Down
4 changes: 1 addition & 3 deletions examples/real-world/store/configureStore.prod.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { createStore, applyMiddleware } from 'redux'
import { syncHistory } from 'react-router-redux'
import { browserHistory } from 'react-router'
import thunk from 'redux-thunk'
import api from '../middleware/api'
import rootReducer from '../reducers'
Expand All @@ -9,6 +7,6 @@ export default function configureStore(initialState) {
return createStore(
rootReducer,
initialState,
applyMiddleware(thunk, api, syncHistory(browserHistory))
applyMiddleware(thunk, api)
)
}
103 changes: 103 additions & 0 deletions examples/real-world/syncHistoryWithStore.js
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, {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaulting to true means that if you hydrate state from e.g. localStorage URL will adjust to that initial state. It can be desirable in development and in production "replay" scenarios but it might be surprising to people who just store some Redux state in localStorage. This is why I'd rather see it opt-in.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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 history.replace there, history will actually emit a new location that then won't match what the reducer saw. One option is to use location.key and transitionTo, but then you end up with reactjs/react-router-redux#164 if you're not careful (and I don't understand the fix there).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 go(-history.length) would be better?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 POPs too.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timdorr I think once you're toggling browser actions in DevTools with adjustUrlOnReplay = true you should not expect Back button to function correctly. I don't really see a big issue with this.

Copy link
Contributor

Choose a reason for hiding this comment

The 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, action: 'REPLACE' to replace the current history stack frame instead of pushing a new one)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 action, but that's much less risky. Thanks!

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally history enhancers don't call listen until after someone calls listen on the wrapped history, but I think this is okay in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what the guarantees with store.subscribe are, but history.listen is expected to synchronously invoke the listener with the current location.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, spot on!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in e271f55.

listener(getLocationInStore(true))
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Should be added after tests are in place.

)
},

// It also provides a way to destroy internal listeners
dispose() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 history.dispose() in case it exists.

Copy link
Contributor

Choose a reason for hiding this comment

The 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()
}
})
}