Make <Link> work with static containers (take two)

[gaearon]

The second attempt to fix #470, thereby unbreaking every project that uses React Redux with React Router. (The first attempt was #3429.)

I’m mostly using the snippets @taion wrote in #3429 (comment) and #470 (comment) as they work great, with minor tweaks to accommodate the overall project code style. I tested this on a few example projects I have, and they seem to work (even with React Redux connect() optimizations which was the goal.)

All existing tests are passing.

@taion expressed a desire for this to be separated from the router code. I don’t disagree but I personally just won’t have the time to maintain it, so I’m not ready to do this. I’m also not 100% sure that this exact approach will not need any changes, and if it does, it would be easier to do this before any of this becomes a public API of any module.

I would really like to get this into 3.0. Any performance downsides from this PR in my opinion are drowned by the performance downsides of people having to turn off connect() optimizations. In a typical app, user actions happen much more frequently on the page than route changes. Given the popularity of both Redux and React Router, even if we personally don’t use them together, I’m sure that lots of React Router users do, and will benefit from this finally not breaking.

[taion]

Would you mind if I pulled the utility out into a library, then? I'd also like to tweak this a bit at the edges to add the discussed fallback for animation-related use cases.

I've been pretty busy lately, so I'm not too sure what else we want to introduce for v3, but I don't think we have any "major" breaking changes left – I'm not even sure about the absolute path stuff any more, especially if we want to support maybe different matching algorithms per-route.

Given that this is a major breaking feature update, should we release a v3-rc from this?

[gaearon]

Would you mind if I pulled the utility out into a library, then?

Absolutely. I was just saying I personally lack the horsepower for this at the moment. 👍

Given that this is a major breaking feature update, should we release a v3-rc from this?

I’d love to see that.

[taion]

Okay, let's leave this PR open for a day or so to see if anybody else has feedback here. I want to pull this into a library before we cut v3-final, but I think cutting an RC before that happens will let us give our users the best experience possible.

[ryanflorence]

Given that this is a major breaking feature update,

What breaks?

[taion]

React-Router-Bootstrap breaks, because it uses Link.prototype.handleClick. 😛

More seriously, anybody using <StaticContainer> or equivalents to preserve link active state during transitions will be broken by this.

[taion]

Sorry, I should clarify – by "major breaking feature update", I mean that this is a major feature update that is also a breaking change, not that this is a feature update that introduces major breaking changes.

[gaearon]

I'd also like to tweak this a bit at the edges to add the discussed fallback for animation-related use cases.

This shouldn’t be blocking though because animation use cases are broken with the existing version too, as they were relying on a React bug that has been fixed.

[gaearon]

Ah, I think I understand now. It was possible to use <StaticContainer> to block those updates during animation, but not now. This makes sense. (Although technically React fixing the original bug, which might happen in a patch release, would also break these use cases.)

[taion]

Yes – the current recommendation is to use <StaticContainer> to keep the old active state, and was in earlier versions of the animations example before we removed it to make it a bit less inscrutable: https://github.com/reactjs/react-router/blob/v1.0.3/examples/animations/app.js#L41-L50.

[ryanflorence]

the name of this isn't clear to me that's it's going to subscribe to some thing with any random name that happens to have a listen method. Why all the abstraction? Why not just get router off of context directly and listen to changes?

[taion]

I want to eventually pull this out into a separate library. There are a number of other React libraries that use context in this sort of pattern, and they all have the same issue of not playing well with Redux and Relay containers that implement SCU optimizations, and I'd like to use this functionality there as well.

[timdorr]

I agree with @ryanflorence here. This doesn't feel like the right place for all of this. I know you want to build out a separate library, but I don't think it's the right place to live-test this approach. We already have a listen and router is on context, so this can probably be simplified quite a bit using existing code.

[taion]

There are some bugs here that I will fix. Would this look better if it actually were a separate library?

[erikras]

Why no decorator syntax? 😢

export function connectToContext(name, contextType = PropTypes.any) {
  return WrappedComponent => {
     const ContextSubscriber = React.createClass({
     })
     return ContextSubscriber
  }
}

To allow:

@connectToContext('theKey')
class MyComponent extends Component {
}

I know it's still experimental and all, but...it's so concise.

[timdorr]

We already have that with withRouter.

[taion]

I didn't think of it. 😛

On the other hand, the subscriber is really only meant to be used in a very few places – not in application code unless working around a library that doesn't do this, so I don't think we need to make as many affordances for DX as on e.g. Redux @connect or something.

[gaearon]

This is not a public API anyway.

[taion]

Not of this library, anyway 😄

[taion]

Tiny correction, I'm thinking v3.0.0-pre.0 instead of v3.0.0-rc.0, until we have a bit more confidence we don't want to sneak in other semver-major changes.

[taion]

Any other feedback? I'd like to cut the prerelease soon.

I'm proposing 3.0.0-pre.0 because I think there might be a few other breaking changes we want to make. The rc should be when we believe we don't intend on making further breaking changes, but might realize that we have to because of things we learn in the wild.

[timdorr]

I'd feel free to publish as many dist-tag'ed next releases as you want. It's prerelease, so it should be volatile 😄

[taion]

Per discussion with @ryanflorence, I'm going to pull this in for now, then make some fixes separately.

[turadg]

FYI this alpha-1 is working for my reasonably large app. Just had to remove this earlier hack https://gist.github.com/gaearon/dfb80954a524709bcaf3bc584d9db11f