-
Notifications
You must be signed in to change notification settings - Fork 114
Possible bug when used with React-Redux v7 #76
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
Comments
Thanks for pointing this out to us! The context value reference is a common class of bug I have seen using hooks now, maybe I should write a PSA on it :) |
Thanks @markerikson for reporting the issue and @abettadapur for fixing it. @abettadapur is the issue still open as we are waiting for a new release, or something else? |
We have been using 4.0.0-alpha.1 for awhile now with React Redux 7. No
issues that I can tell. We should publish 4.0.0
…On Wed, May 22, 2019, 08:04 Navneet Gupta ***@***.***> wrote:
Thanks @markerikson <https://github.com/markerikson> for reporting the
issue and @abettadapur <https://github.com/abettadapur> for fixing it.
@abettadapur <https://github.com/abettadapur> is the issue still open as
we are waiting for a new release, or something else?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#76?email_source=notifications&email_token=AASNYNJTESS6PO4WRRWJHFTPWVOIPA5CNFSM4HEBIYCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV7LIDY#issuecomment-494842895>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASNYNO6723TLFOF3BJVFB3PWVOIPANCNFSM4HEBIYCA>
.
|
Thanks @abettadapur ... I assume we don't need to port anything to 3.*. I will publish 4.0.0 today. |
redux-dynamic-modules-react
currently has this chunk of code:https://github.com/Microsoft/redux-dynamic-modules/blob/8d9bbbbb037abeae2b3f4f62345668097c0f8015/packages/redux-dynamic-modules-react/src/DynamicModuleLoader.tsx#L141-L150
This was related to a bug that popped up in React-Redux v7 beta 0, at reduxjs/react-redux#1219 .
Looking at the code further, I can see that you're rendering a
<ReactReduxContext.Provider>
with your own value. By doing that, you're actually going to completely override the context value from React-Redux itself. That context value normally looks like{store, subscription]
in v7. Based on your code, thesubscription
value will be lost, and nested connected components will not be updated in the correct sequence, and will not take advantage of React's batched updates.I'd suggest changing this chunk to render both a
<ReactReduxContext.Consumer>
and a<ReactReduxContext.Provider>
, pass down a context value that looks like{store, storeState, subscription}
, and memoize producing that value so that it doesn't keep changing on every render (to avoid unnecessary context-caused updates).The text was updated successfully, but these errors were encountered: