Make <Link> work with static containers (take two) #3430
Conversation
|
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? |
Absolutely. I was just saying I personally lack the horsepower for this at the moment. 👍
I’d love to see that. |
|
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. |
What breaks? |
|
React-Router-Bootstrap breaks, because it uses More seriously, anybody using |
|
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. |
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. |
|
Ah, I think I understand now. It was possible to use |
|
Yes – the current recommendation is to use |
| return ContextProvider | ||
| } | ||
|
|
||
| export function connectToContext(WrappedComponent, name, contextType = PropTypes.any) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There are some bugs here that I will fix. Would this look better if it actually were a separate library?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We already have that with withRouter.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is not a public API anyway.
There was a problem hiding this comment.
Not of this library, anyway 😄
|
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. |
|
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. |
|
I'd feel free to publish as many dist-tag'ed |
|
Per discussion with @ryanflorence, I'm going to pull this in for now, then make some fixes separately. |
- only re-render component if props or requests shallowly changed - this.setAtomicState now creates new objects for all 4 keys - added tests for `pure: true` and `pure: false` - added `pure` option to api docs
* Also upgrade react-router-redux * Pin react-redux to version 4.4.3 to avoid issue described in remix-run/react-router#3430. That issue is targeted for react-router 3.0, so we can un-pin once that is released.
|
FYI this alpha-1 is working for my reasonably large app. Just had to remove this earlier hack https://gist.github.com/gaearon/dfb80954a524709bcaf3bc584d9db11f |
ghost
mentioned this pull request
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.