Alternative Proposed Solution For React-Router + Redux Integration #248
Description
(tl;dr - maybe we should use the RouterContext
from React-Router v2 for syncing the redux-store? We could get routing-connected components without infinite loops and router props in the store!)
For starters, I've been really impressed with goals and results that this project has displayed in such a short time. Moving from syncing the pathname to the entire location descriptor as well as improving the support for the devtools while keeping the API super simple and close to React-Router has been a big win for both the users and the maintainers (or at least it appears to be from my end).
Tying to history directly as the source for updating the store and vice-versa makes this project very flexible (you don't even need React-Router technically), but I think results in some negative consequences:
-
Connecting to the Routing State is Dangerous
With syncing with history, you must discourage users from doing what seems natural in a redux setting and connecting the router state to their components (Infinite loop when dispatching redirect from within a componentWillReceiveProps #212) leading to the following in the README:
Warning: It is a bad pattern to use react-redux's connect decorator to map the state from this reducer to props on your Route components. This can lead to infinite loops and performance problems. react-router already provides this for you via this.props.location.
I think this is due to the fact that the router props and the redux-store are not from the same source (only indirectly via history) and therefore are not guaranteed to update in sync.
-
Store omits the React-Router Props
The second limitation of the history-redux approach is missing out on React-Router's params in the redux -store, the source of confusion and many feature requests (passing params from react-router #246, Could use fuller explanation of project vision #228, etc)
I know that you can use the props directly from React-Router, and I've responded to several issues and discord questions with that very advice. I would be happy with these tradeoffs for simplicity if there were not another solution.
An Alternative using RouterContext
I think that React-Router 2.x actually provides a best of both worlds approach where we can get an easy API which matches React-Router, maintains a small codebase, and solves the two limitations above. I actually stumbled across this while trying to see if there was a way to upgrade the old redux-router (before this project became part of rackt and added the location object sync).
Here's an alternative using the RouterContext
from React-Router 2. The central idea is that the props passed to the RouterContext
are directly from the store, therefore a component below this ReduxRouter
cannot ever get a property from the router that is different than the store's data. The ReduxRouterContext
is responsible for managing and resolving differences between the store and the props passed down from ReactRouter
function routerPropsFromProps(props) {
return {
location: props.location,
params: props.params,
};
}
@connect(
(state, { routerStateSelector }) => { return { routerState: routerStateSelector(state) }; },
{ routerDidChange }
)
class ReduxRouterContext extends Component {
static propTypes = {
router: PropTypes.object,
routerDidChange: PropTypes.func.isRequired,
routerState: PropTypes.object,
}
componentWillMount() {
// On mount, sync to the store to React-Router props
this.props.routerDidChange(routerPropsFromProps(this.props));
}
componentWillReceiveProps(newProps) {
// New routing props from React-Router and it doesnt match our store, Update the store
if (!routerStateEquals(routerPropsFromProps(newProps), routerPropsFromProps(this.props)) &&
!routerStateEquals(routerPropsFromProps(newProps), newProps.routerState)) {
this.props.routerDidChange(routerPropsFromProps(newProps));
// New store state and it doesnt match the next routing props, transition via the router
// This is common when replaying devTools
} else if (!routerStateEquals(newProps.routerState, this.props.routerState) &&
!routerStateEquals(routerPropsFromProps(newProps), newProps.routerState)) {
if (newProps.routerState) {
newProps.router.transitionTo(newProps.routerState.location);
}
}
}
render() {
const { routerDidChange, routerState, ...others } = this.props;
const newRouterProps = {
...others,
...routerState, // Override react-router's props with the store routing props
};
return <RouterContext {...newRouterProps} />;
}
}
class ReduxRouter extends Component {
render() {
return (<Router render={props => <ReduxRouterContext {...props} />}
{...this.props}
/>);
}
}
I omitted a small wrapper class which facilitates in memoization and passing down the state selector, as well as the routerStateEquals which would compare location objects (maybe omit the key if users dont want a re-render on same page loads #222?). You would also add a small middleware for history so users can do the routeActions.push
and other history navigation from redux which would just call history directly. Also of course would be a reducer to store the data from routerDidChange
.
The use for this would be a (really) minor tweak to how users interact with the current React-Router API.
Basically anywhere a user would use <Router>
they use <ReduxRouter>
(or ReactRouterRedux 😄 ) or if server rendering, use <ReduxRouterContext>
instead of <RouterContext>
.
Feel free to peruse the actual source at https://github.com/mjrussell/redux-router/tree/rr-v2 with tests and the examples (basic and server rendering) originally from redux-router updated.
The Way Forward
I'd love for this (way too long) post to be a discussion about the tradeoffs of an approach like this compared to the current implementation/direction. Maybe this implementation is completely flawed and should be thrown out, or there is a similar approach but with some tweaks. Or, this is what Redux-Router should evolve into and then maintain a 2 redux-routing approach in the ecosystem.
Thanks and keep up all the great work!
Activity
timdorr commentedon Jan 30, 2016
The new
render
API in react-router 2.0 was basically built for this kind of thing, so it's a sensible approach.Did you want to put together a PR for how to implement this?
gaearon commentedon Feb 1, 2016
👍 for this, in a way this is what Redux Router wanted to be
mjrussell commentedon Feb 1, 2016
@timdorr @gaearon thanks for the support! I'll clean it up and get a PR in soon. I already have most of the work done in that Redux-Router fork so it should be fairly easy.
jlongster commentedon Feb 1, 2016
I'd like @taion to check this out, he wrote the current strategy and he seemed to think that 2.0's API didn't have much impact on this (I wish I could find that comment, but no time right now).
Trying to wrap my head around what will happen if the redux state changes during the
routerDidChange
action incomponentWillReceiveProps
... When are connected components rendered?I also don't see anything for syncing the redux store -> router. We need
push
,replace
, actions that are redux actions but end up updating the router. This looks like all it solves is router -> store, and seems more complicated than our current implemented without any gains to the user (unless I'm missing something?) Now the user has to use different tags, but that's not a huge deal, if that's how they interface instead of callingsyncHistory
.taion commentedon Feb 1, 2016
I'd say "it depends". I think the current implementation is a good way to implement something like the original redux-simple-router that just plugs into history, and has the benefit of being nice and simple.
However, if you want access to
params
, and if you want the update to the store to reflect the routing state rather than the history state (e.g. in case there are async routes), then integrating into just thehistory
is not enough.That said, it's not clear to me that
render
is the right way to go about this. I think that if you want to do this sort of thing, you really do want something like the actual redux-router approach, where the Redux store holds the routing state, and you replace<Router>
with... well, effectively just aconnect()
. This seems much cleaner to me than dispatching an action incomponentWillReceiveProps
of a custom<RouterContext>
– the latter API was meant much more for e.g. async-props or react-router-relay, which want to control route components more than router state itself.I have neither the time nor the expertise to really understand redux-router right now, but can we go over at a high level what the problems there were?
mjrussell commentedon Feb 1, 2016
Yeah I'd love to get more opinions on this before spending time wrapping into a PR. Although if the PR format proves better for discussion (inline comments/easier updates) Im happy to do that anyway.
Yeah I made a small comment but it could easily have gotten lost in that wall of text. You'd want to include some sort of historyMiddleware such as the one I used. Essentially this section of the current middleware.
I think the gains I see in this approach are that you can now use
connect
with the routing store data without worrying about the router params being different. By telling users to only use the params, it doesn't really feel like true redux. You'd be better off hiding this data inside the middleware than letting the users touch it and shoot themselves in the foot (except for the dev tools support...)timdorr commentedon Feb 1, 2016
One thing to remember about router state is it is read-only. We don't do something like
RouterParams.set('user_id', 123)
to manipulate the router. We only push locations through history (which are now wrapped in a transition manager so router can hook into history without modifying it), even if the API appears to be telling us we're using the router (this.context.router.push
).So, we don't need to do
redux -> router
because we never tell the router what to do directly in any context. Hence the simplicity of this library.taion commentedon Feb 1, 2016
Sure – you keep the same actions you have right now, but instead of listening to the
history
directly, you listen to the transition manager (or router or whatever – might need some exposed APIs). You leave that state in a store, and then replace<Router>
with something that has its state live in the store rather than in component state.Sounds easy enough on paper; why didn't this work for redux-router?
mjrussell commentedon Feb 1, 2016
I also tried just upgraded redux-router to react-router v2 while preserving the majority of its old functionality see this branch. I found myself copying functionality directly from React-Router into Redux-Router. i.e having to create my own transition manager, my own router object, etc, in order to pass the same things down to router context that also looked similar.
I think the downfall of such an approach is in how difficult it becomes to maintain and possibly depends on internal workings of React-Router. You end up with the same logic and can't benefit from the bug fixes and the community support. Maybe there would be a way to get one level higher on top of
<Router>
instead of letting history pass the data down, force it to go through a connected component first?timdorr commentedon Feb 1, 2016
@taion redux-router essentially monkey patched in
render
before that was an official API. Hence, it was pretty heavily coupled to the router internals. That was probably the worst of it.jlongster commentedon Feb 1, 2016
This feels more like a redux-router v2 (or v3, whatever the next major one is) honestly, and a PR should be opened on redux-router. I really like not accessing router params through redux because it forces you to just use react-router the way it was built (and, of course, reduces complexity in this library). All systems have compromises and we shouldn't get caught up the ideal of redux and doing something "just because it doesn't feel redux-y".
taion commentedon Feb 1, 2016
That is the correct approach though. If you want the Redux store to contain the current routing state, then it should feed
<Router>
, and you will have to maintain a fork of<Router>
. It's not that big a deal, though;Router.js
is maybe 100 SLoC ex-backward-compat shims.This is orthogonal to
render
being an API – it's not about replacing<RoutingContext>
or<RouterContext>
, it's about replacing<Router>
while keeping the context.taion commentedon Feb 1, 2016
Like what the heck even is all this stuff? https://github.com/acdlite/redux-router/blob/v1.0.0-beta6/src/routeReplacement.js
mjrussell commentedon Feb 1, 2016
Its one of the wish-list features of React-Router 😄 replacing routes dynamically! Agreed, stuff like this made it very hard to maintain.
timdorr commentedon Feb 1, 2016
@taion Why would we want to reimplement any of
<Router>
?render
is a middleware system of sorts. It's basically built for this.43 remaining items