Alternative Proposed Solution For React-Router + Redux Integration

[mjrussell]

(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:

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

2. 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!

[timdorr]

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]

👍 for this, in a way this is what Redux Router wanted to be

[mjrussell]

@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]

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 in componentWillReceiveProps... 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 calling syncHistory.

[taion]

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 the history 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 a connect(). This seems much cleaner to me than dispatching an action in componentWillReceiveProps 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]

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

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.

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.

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.

seems more complicated than our current implemented without any gains to the user

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]

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]

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]

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?

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]

@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]

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]

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]

Like what the heck even is all this stuff? https://github.com/acdlite/redux-router/blob/v1.0.0-beta6/src/routeReplacement.js

[mjrussell]

Like what the heck even is all this stuff?

Its one of the wish-list features of React-Router 😄 replacing routes dynamically! Agreed, stuff like this made it very hard to maintain.

[timdorr]

@taion Why would we want to reimplement any of <Router>? render is a middleware system of sorts. It's basically built for this.

[timdorr]

Also, the only thing people really want out of this is props.params. No need to bother with transition managers and router objects just for that.

[taion]

@timdorr

render() is about controlling how you render your route components – for example, automatically wiring them up to store-like state in async-props, or else connecting route components to Relay in react-router-relay.

This is doing the opposite – this is moving the contents this.state on <Router> into a Flux store; better to have that be a single source of truth, since you can just replace <Router>, whereas no such replacement is possible for the current implementation.

@mjrussell / @jlongster

If that's the main issue, then I think a very restricted version of redux-router would still be true to the original redux-simple-router ideal.

Instead of history.listen, you do transitionManager.listen, and you feed it into a copy/paste of <Router> that just gets its state by connecting to the store.

[taion]

A correct API should insulate all of that from users. It should look largely like the current API, except with a <ReduxRouterContext> living under <Provider> rather than a <Router>, which would no longer be needed since all the state management would be owned by Redux. It would also make navigation replay much less broken.

Setting up a custom router context just to dispatch an action in cWRP smells very wrong to me.

[jlongster]

My #1 complaint of redux-router was that it stored the router's literal state in redux, which as all sorts of things like component instances. You really, really do not want to do that because it's not serializable, so you kill any ability to record/replay across sessions, etc. Or even simply viewing a log becomes hard. Only plain JS objects (or some agreed upon data structure like immutable.js) should live in redux state.

So my only constraints are: only keep plain objects in the store, and keep this library <200 lines of code :D If we can satisfy those rules I don't really care how we do it. (The 200 lines is a joke, but keeping this small is a good indicator to me that we're still letting react-router do all the work)

[timdorr]

@taion No, it's not solely about that. Any middleware gets both input and produces an output. We can use that input and just pass through an output:

<Router render={props => (reduxRender(props, <RouterContext {...props} />))} />

We don't need all of this.state in the router. That contains lots of useless data that is normally exposed to route components anyways. All people care about here are props.params.

[timdorr]

@jlongster Yeah, that's what I'm saying. We don't need all of this.state. It's not normally exposed to route components, so why would anyone need it in their redux store? If they do, they're not using router correctly 😄

[taion]

I really don't like the idea of putting just params in the store. Unlike location, params are not meaningful in a vacuum – they're with respect to the matched routes.

Having a read API existing outside of components that exposes params but not also routes or the equivalent seems incorrect to me.

If serializability means we can't put the routing state in the Redux store, then we shouldn't put route params in there either, to avoid anti-patterns there, and just restrict route params to be accessed via the standard React Router API.

[timdorr]

The route component is only used within the context of an active route and it should be aware of what it represents. Yes, the params are practically useless without a route to give them context, but that context is created by the route component. It's roundabout, but it makes that connection.

Users just want to do this connect(state => ({ userid: state.routing.params.user_id }))(UserProfilePage). It feels weird to tell them not to use Redux for something relatively core to the library, conceptually speaking. Otherwise, we should rename this to redux-history 😳

[gaearon]

If this library explicitly wants you to handle routing via Router props, how about we just hide the routing from the state? It’s doable with a store enhancer (just like DevTools hide their internal state).

[taion]

@gaearon

I like that! My one concern is that it would break the pattern of doing e.g.

push({ ...location, query: { the: 'query' } })

to change just a single component of the location.

[gaearon]

How do people usually read that location with vanilla React Router? We can encourage using the same way.

[gaearon]

We can allow stuff like

push(location => ({ ...location, query: { the: 'query' } }))

if we want to.

Similar to how setState can accept function as an argument.

[gaearon]

@taion If you wanna chat about this hop onto Discord. I wanted to help out today, and I’m happy to make bigger changes on top of #255 if they make sense to y’all.

[jlongster]

I would be interested in seeing how various people use the routing state and make sure there is an appropriate alternative. I have a feeling there might be some use cases in action creators that involve reading the current location. I suppose React components could just pass in the location from the component though...

[gaearon]

I suppose React components could just pass in the location from the component though...

This seems more in line with what this library is supposed to do (“let RR do all the work”).

The user won't have to mount the reducer or provide selectLocation. Hiding state.routing and making this a store enhancer will allow us to be more flexible with implementation without breaking everyone again. (Since store enhancers can do pretty much anything, unlike middleware.)

[SimenB]

I like being able to read state (and I look at the URL, and it's parsed params, as state) from the single redux store. But I also have no particular problem passing that as props from routing components, I don't need that part of the state so often it's a problem in practice.

The main benefit of this library I think is being able to have URL managed by redux, meaning dev tools, restoring state and changing the url through actions. And as long as it does that, putting thing in an accessible part of the state is just convenience, IMO, particularly when it's accessible as props in routing components.

[gaearon]

I like being able to read state (and I look at the URL, and it's parsed params, as state) from the single redux store, but I also have no particular problem passing that as props from routing components

The non-obvious thing is that if you use React Router, routing state in components and state in the store can be different. This is why I want to remove routing state from the (visible part of the) store completely.

Maybe I can try creating a new proof of concept project that doesn't give you any reducers, action creators, etc, and whose only goal is to export a store enhancer that magically “hides” history state so persistence and DevTools “just work”. But you’d use React Router and History for pushing / reading the state, so effectively, except for the store enhancement, it would have no public API.

[edygar]

I've been using as exposed here: #251 and for me it is working very well…

[VinSpee]

I wrote a middleware to handle analytics and am using it to get the current path:

if (isRoutingAction(action.type)) {
  newAction = {
    action: action.type,
    transitionType: action.payload.action,
    formStep: getState().routing.location.pathname,
  };
}

[timdorr]

If you remove state.routing, what is the point of this library? Outside of being able to push actions from anywhere to update the location (which you can do with the history singletons in router 2.0), it seems relatively pointless to add this to a project.

[gaearon]

If you remove state.routing, what is the point of this library? Outside of being able to push actions from anywhere to update the location (which you can do with the history singletons in router 2.0), it seems relatively pointless to add this to a project.

I thought this is the point (at least the one I care about):

Redux is awesome. React Router is cool. The problem is that react-router manages an important piece of your application state: the URL. If you are using redux, you want your app state to fully represent your UI; if you snapshotted the app state, you should be able to load it up later and see the same thing.

I don't think value proposition of exposing state.routing is high enough. Maybe we should make it more convenient to read current location from history object instead? Because this is basically what users want when they read state.routing. And inside components, they can just use router props.

[VinSpee]

@gaearon but what if it's inside of a middleware?

[timdorr]

@gaearon But if you hide that state, when you go to snapshot it, it's not there. It's got to be visible for that part of the vision to be realized.

[gaearon]

Access the history object inside of the middleware.

[gaearon]

@timdorr No, this is possible to do via store enhancers. It's exactly how DevTools state (of the tools themselves!) can be serialized and persisted without the app knowing.

[gaearon]

I can whip up a proof of concept today and you’ll let me know if you’d like to take it further :-)

[timdorr]

Yeah, but if it's hidden from the world outside of that store enhancer, how would Dev Tools be able to see it and not everyone else? The store enhancer doesn't know if it's being passed off to another enhancer or just the vanilla createStore, so I'm not seeing how that would happen. This library could serialize itself, but I don't think that's the goal here.

[gaearon]

In DevTools we override getState() to return state.appState and wrap the root reducer into another reducer that delegates appState to your reducer, but also has its own reducer for its internal state. I'm not sure whether this approach would work here but it's worth a try IMO :-)

[taion]

In a perfect world, Redux would own the router state entirely, but that won't work because of serializability.

The next-best approach might be to have Redux own the history state (#138), except that won't work because the browser owns the history state, and there just isn't enough fine-grained control there.

Except...

We can get an ersatz version of #138 – for replay, just set up a history enhancer that listens to updates on the store. When the store updates, if it's to a new location that the history doesn't know, then the history can just re-publish that new location to all of its listeners.

This gives a much more seamless approach to making replay work. Syncing the actual URL will take a bit more work, but it gives you location state updates much more nicely.

[timdorr]

@taion I like that approach. We would have no middleware, just actions that get dispatched directly into the store. A subscriber to attempt to update history when state.routing changes. And a history.listener to dump more changes into state as they occur.

My only question is how navigation cancellations are handled by history. This is just my lack of familiarity with that bit of the code, but does a transitionTo cancellation give us feedback in some form to be able to update the store back to the previous location? That could be the only hiccup I see with this.

[timdorr]

BTW, it looks like it does. I just wanted to be sure.

[gaearon]

We can get an ersatz version of #138 – for replay, just set up a history enhancer that listens to updates on the store. When the store updates, if it's to a new location that the history doesn't know, then the history can just re-publish that new location to all of its listeners.

This sounds sensible. I was also thinking about this approach today. It would be very cool to just do that, and free react-router-redux from the responsibility of handling replays.

That said “you should not access state.routing” still does not make sense to me. :-( If it’s not a reliable source why don’t we just ask people to get current location from the router or history?

[mjrussell]

That said “you should not access state.routing” still does not make sense to me. :-( If it’s not a reliable source why don’t we just ask people to get current location from the router or history?

I completely agree, either the state.routing should always be in sync with react-router props or it shouldn't be present at all. It invites users to cause themselves pain.

[taion]

state.routing absolutely will not be in sync with React Router props if there are async routes, and there's very little we can do about it unless we inject Redux between the transition manager and the <Router> (since we can't just put the router state in Redux entirely).

I think the approach we should take is to track the routing state internally for purposes of replay; we shouldn't expose the reducer any more (and it won't be required any more), but we probably shouldn't e.g. hide UPDATE_LOCATION actions.

[timdorr]

@gaearon I thought you wanted to not provide state.routing? I'm getting confused here :)

@taion state.routing doesn't (yet) connect to router state, just history. It's really just state.routing.location. The extra level is so that we can, in theory, provide state.routing.params at some point.

[gaearon]

@timdorr

Woah, sorry, I phrased this badly. What I meant is the current model of “it’s there but please don’t access it” is broken. Either it should be up-to-date (can’t be with async props), or we should remove access to it, or we should keep the access but somehow make it clear this state is not what you want to retrieve in the components.

[gaearon]

I’ve got a working proof of concept proposal of how I see this library working in the future: reduxjs/redux#1362. Please let me know if I missed something.

I’m happy to submit a PR here if you folks agree with this approach.

[timdorr]

In light of the (bunch of) discussions based on @gaearon's ideas and work, I'm going to close this one so we can focus on the work in #259.

[mjrussell]

Yeah agreed, got the spirit of what I was hoping for without the render middleware!