Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shouldComponentUpdate() is ignored when Legacy Context and New Context are co-existing #13856

Closed
stomita opened this issue Oct 16, 2018 · 6 comments

Comments

@stomita
Copy link

stomita commented Oct 16, 2018

Do you want to request a feature or report a bug?

bug.

What is the current behavior?

In a component tree where legacy context and new context are co-exisiting, when components are placed on tree nodes under the legacy context provider and between new context provider and consumer, components are always rendered even if shouldComponentUpdate() returns false in a parent component.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem.

https://jsfiddle.net/fdrgz9c4/15/

The above is the minimal case I've created without other dependencies, but first I found this behaviour in my Redux application, which uses legacy style context to map the state in store.

What is the expected behavior?

Should not call render() when shouldComponentUpdate() returns false in parent components, as same as the other cases (it prevents rendering when it is located without legacy context or not between new context provider / consumer tree. See the above fiddle)

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.5.2, maybe in older versions.
No OS/browser dependencies I believe.

@gaearon
Copy link
Collaborator

gaearon commented Oct 16, 2018

There are a few bugs like this but they are rather difficult to fix and would increase the code size. While this may not be satisfactory our recommendation is to migrate away from legacy context when you can. If you can look into fixing this it would be great though.

@stomita
Copy link
Author

stomita commented Oct 16, 2018

@gaearon As the react-redux is still using legacy context it becomes difficult to migrate fully to the new context. If the fix of this issue is difficult, I might continue using legacy context in my app, at least until react-redux is introduced with new context API.

@WOLVIE97
Copy link

KK....💋... Well shit then.
.. yeah still here...

@kafein
Copy link

kafein commented Oct 16, 2018

I faced same issue with react-router. Thanks to @mjackson, it's now gone. I think same approach can used to fix react-redux, if they are going support legacy context in future version.
remix-run/react-router#6348

@markerikson
Copy link
Contributor

@stomita , @kafein : we're working on a new React-Redux version 6 that switches to use createContext instead of legacy context. Please see reduxjs/react-redux#995 and reduxjs/react-redux#1000 for our work-in-progress PRs, both of which are available on NPM as tags next-995 and next-1000 respectively. I've also been fiddling with some more tweaks that I hope may be worth pushing as updates in the near future.

I'd appreciate it if you could try out those builds and see how well they work overall, and give us feedback in those issues.

@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2018

Going to close as most likely duplicate of #11508.

@gaearon gaearon closed this as completed Nov 1, 2018
@markerikson
Copy link
Contributor

We've just published React-Redux 6.0 beta 1, which uses createContext internally. Please try it out and let us know how it works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants