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.