Make withRouter() update inside static containers #3443
Conversation
| if (eventIndex !== this.state[lastRenderedEventIndexKey]) { | ||
| if (this.context[name] !== nextValue) { | ||
| // React uses a stale value so we update it manually. | ||
| this.context[name] = nextValue; |
There was a problem hiding this comment.
I’m not proud of this but it works fine. I’ll keep an eye on this not breaking in the React repo 😄
| @@ -43,8 +43,9 @@ export function ContextProvider(name) { | |||
| }, | |||
|
|
|||
| componentDidUpdate() { | |||
| const nextValue = this.getChildContext()[name] | |||
There was a problem hiding this comment.
This is safe because React makes no guarantees about when this method can be called
|
I think on net this is a bit messier than the earlier implementation that just updated the router object in-place. |
|
Yea. On the other hand, this keeps the desired property of |
|
I mean, it's an unfortunate caveat to have, but I'm guessing it's probably slightly more performant to not recreate the object every time. Having to keep the same object identity is a caveat that I can live with. |
gaearon
force-pushed
the
with-router-static
branch
from
a39d041
to
bb3e2df
Compare
I think this would only be relevant if navigation was something that’s performed hundreds of time per second. I wouldn’t expect this to have any measurable performance in this particular scenario. Happy to change it though. You sure? |
|
What I like about this is while it’s messier on the net, the messy bits are all in the “polyfill”. As opposed to spreading through the legit code. |
|
I’m fairly content with what I posted before but I added 893cbf0 as a minimally invasive alternative approach. It adds |
gaearon
force-pushed
the
with-router-static
branch
from
7b3abdb
to
893cbf0
Compare
|
cc @jquense who's done some stuff to work around this issue. At this point I'm really torn. |
|
Ultimately we can go either way and switch if we find problems later. I’d probably use the last commit because it doesn’t touch |
|
Yeah, I'm fine with imposing a bit of extra work on the users of this polyfill I think. Probably I'd be happier mutating my context objects in place rather than worrying that setting |
|
In case we want to change the mind later, the test case I added should catch any mistakes. |
|
I'm happy with this for now. |
|
@taion One last problem is this still doesn’t update containers with shallow equality check. We can either revert the last commit or add a bogus prop just to force an update. |
|
Alternatively, we can inject |
|
You mean in case someone does |
|
Yeah. That’s the primary use case I’m trying to get support for because it’s very common in Redux apps. |
|
I like the idea of |
Fixes #3439.