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.