React-router: Make withRouter() update in response to context changes

[gaearon]

I wonder if you’d be up for accepting a PR like #3430 but for withRouter() (#3352). This would eliminate the problem of “how do I connect to the router state in Redux in a deep component” because people would just wrap their deep components with withRouter and be guaranteed to get the right params.

If this makes sense to you, I’d submit a PR that moves the context fix to be used by withRouter, and change Link to use withRouter internally.

[taion]

Do we want <Link> to use withRouter? It does make the component hierarchy a bit deeper for maybe not so much benefit? But then again the consistency would be nice. Definitely withRouter should have the context skip logic though.

[timdorr]

Yeah, that fix probably should have been done to withRouter and we should have carried it downstream to <Link>.

@taion beat me to it, but the only issue I see is adding another layer to every <Link> in the wild. (Side note: It would be nice if you could extend the React class to make a HoC, rather than wrapping it) Not a huge deal, and it means better code reuse in the project (more dogfooding of the new HoC).

[gaearon]

@taion beat me to it, but the only issue I see is adding another layer to every in the wild

We could change the context helpers to be React old-style mixins. This would remove the need for extra wrappers in both cases. Since they are not public API I think it’s a sensible compromise.

[taion]

To clarify, I don't have a strong opinion here – if you feel that it's better to have a single withRouter HoC-based codepath and use that in <Link>, I'd be okay with that.

[timdorr]

The issue isn't the context helper code, it's adding the withRouter wrapper to <Link>. That adds another layer.

[gaearon]

If we make context helpers mixins, we can use them both from withRouter and Link independently.

[gaearon]

The problem is that we already added extra layer to Link and root component in #3430, and it feels wrong to do it again for every withRouter consumer, as this would cause any withRouter to be two levels deep. Using mixins as implementation detail lets us flatten this hierarchy.

[taion]

Unless we want to use mixins, we're already adding an extra layer for the context subscriber. I think I'd go with the extra HoCs over using React mixins. At least start like this and see if people complain about the extra wrappers? Should be easy enough to cut things over to mixins if the extra elements become an issue.

[gaearon]

The upside of using mixins is less deep tree. People have already been complaining about the bunch of stuff at the root that makes navigating React / Redux / React Router apps difficult in React DevTools. We don’t have to make it worse.

Generally mixins can be problematic from maintenance point of view but to me if feels like the exact situation where they solve a problem elegantly. When React fixes context, we can drop those mixins and forget about them, and consumers don’t have to dig through deeper trees just because of that issue.

Those mixins are also never leaked to public API.

Why do you view using mixins for this as problematic?

[timdorr]

Why do you view using mixins for this as problematic?

This project has been moving away from them for the last few releases. They are entirely gone in 3.0. I think it's just an aversion to something that's viewed as being "in the past". ES2015 classes have gained prominence now that browser support is coming online and they are view as "the future".

[gaearon]

I don’t think I strike anyone as a proponent of mixins though 😄 . Mixins introduce implicit dependencies that make the project hard to refactor. This is why I don’t recommend using them in apps, and I understand why RR wanted to get away from them internally.

This case is special because those mixins aren’t a way to share data or add functionality. It’s just a way to patch a bug in React with a workaround that can be cleanly removed when it’s unnecessary. Fixing it with mixins is less invasive than with HOCs. HOCs are useful to explain the data flow _but there is no data flow here: we’re just patching over a bug_.

This is why I think making an exception in this particular case is completely worthwhile.

[gaearon]

I think it's just an aversion to something that's viewed as being "in the past". ES2015 classes have gained prominence now that browser support is coming online and they are view as "the future".

FWIW, React doesn’t plan to drop support for mixins any time soon, so this is purely psychological. If we see them as solving a use case, we can use them.

[timdorr]

Oh yeah, I don't think they're going anywhere. It's the typical JS dev's way of thinking: "If I don't have a blank npm outdated output, I must be doing something wrong!" 😉

What if connectToContext was renamed to buildContextClass that returned just the prototype object for withRouter to make it's own class? Then part of withRouter would end up looking something like this:

const contextClass = buildContextClass('router')
contextClass.render = () => (<WrappedComponent {...this.props} router={this.context.router} />)
const WithRouter = React.createClass(contextClass)

[taion]

I think mixins are fine here if we want to keep using React.createClass. The other option would be to move over to classes and use a prototype-mutating HoC (or an HoC that creates a new class by merging stuff into the prototype, &c.)

I agree that we don't want to see:

<ContextSubscriber(WithRouter(Link))>
  <WithRouter(Link)>
    <Link />
  </WithRouter(Link)>
</ContextSubscriber(WithRouter(Link))>

But to go back to the original question, do we want to have

<WithRouter(Link)>
  <Link />
</WithRouter(Link)>

I'm still (marginally) inclined toward "no" on the latter.

[gaearon]

I might be wrong but I think you’re reinventing mixins 😄 .

They already do a ton of useful stuff for our use case. For example, they merge lifecycle methods and state so mixins can do their hacky thing with componentWillReceiveProps, their own internal state, context subscription and child context declaration without burdening the actual implementation.

[taion]

Not exactly my re-invention 😛 wondering if we wanted to move off of React.createClass.

[gaearon]

But to go back to the original question, do we want to have

<WithRouter(Link)>
  <Link>
</WithRouter(Link)>

I'm still (marginally) inclined toward "no" on the latter.

I agree, I’d keep it just a <Link>. As long as they both use those mixins independently so both get deeply updated when necessary.

[taion]

withRouter can't be a mixin, though, unless we want to do something really nasty with mutating this.props.

[gaearon]

@taion

Sorry, I was referring to https://github.com/reactjs/react-router/issues/3439#issuecomment-217892198. I’d say writing any custom merging code is brittle and we’d have to remember about it. On the other hand, React mixins already know how to merge all the lifecycle methods correctly. So I don’t see the point.

wondering if we wanted to move off of React.createClass.

For these two helpers, I think staying with it is fine.

withRouter can't be a mixin, though, unless we want to do something really nasty with mutating this.props.

The class _generated_ by withRouter can.

function withRouter(WrappedComponent) {
  return React.createClass({
    mixins: [ContextSubscriber('router')], // remove this line when React fixes context

    ...

   render() {
     return <WrapperComponent {...this.props} router={this.context.router} />
  })
}


var Link = React.createClass({
  mixins: [ContextSubscriber('router')], // remove this line when React fixes context
  ...
})

[timdorr]

I don't think we need withRouter for Link. It's nice to dogfood, but that's only for our benefit. For the user, we can just use the same ContextSubscriber mixin for each tool.

[gaearon]

@timdorr I agree. I’m not using it for Link in the comment above.

[timdorr]

No, but @taion and I were talking about wrapping Link with it. I don't think it's needed now.

[gaearon]

Agreed, I thought this might be nice in the beginning but I don’t think that anymore. Let’s keep things flat.

[gaearon]

To sum up, my proposal is the following:

• Change ContextProvider and ContextSubscriber to be name => Mixin functions.
• They are temporary hacks so using mixins is fine, and the merging behavior is convenient so it’s easy to kill them later.
• This forces that we keep using createClass for Link, component returned by withRouter, and RouterContext. I don’t think it’s a big deal. And they use createClass now anyway.
• When React fixes the context issue, we can remove this code just by removing the mixins from the code.
• We should at least put params and location on the router object in the context to make it useful for withRouter (#3325)

This is the missing piece to making React Router work with Redux really well. The hierarchy is also flatter which makes debugging easier and performance marginally better.

[timdorr]

This forces that we keep using createClass for Link, component returned by withRouter, and RouterContext. I don’t think it’s a big deal. And they use createClass now anyway.

We actually made the decision to switch back to createClass a while ago, so this aligns with the current project code style anyways.

[taion]

I'm on board with that. I was going to spend some time later today to pull out the context hack stuff (so I could use it in other libraries) and I can move stuff to mixins, but I'd be fine with someone else mixin-ifying the helpers too 😆

[gaearon]

I’m a bit busy today so I’d appreciate if you did it as part of the transition.
I can take a stab at PR-ing #3325 later (it’s essential for my use case as well) but I’m not sure where to “enrich” the router object—<Router> or <RouterContext>.

[taion]

I don't know either :smile: otherwise I would have done it already. I think it might have to be in <Router>, though, because we want to expose a router object to router context middlewares.

[gaearon]

Please see #3440 as first part of this.

[gaearon]

And #3442 as the second part.

[gaearon]

3443 is the last part that closes the issue.

[gaearon]

Let’s cut 3.0.0-alpha?

[timdorr]

@gaearon See #3445

[gaearon]

@timdorr Missed it, thanks!