- Notifications
You must be signed in to change notification settings - Fork24
memoize the wrappedReducer function#19
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for this! Can you add a quick test to demonstrate the fixed behavior? |
chrisdhanaraj commentedAug 19, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Okay, so this is somewhat of a goof on me. My solvedoes fix the problem I was having, but it could also be fixed in user land. I wrote a test for this and did some variations and saw my error - here's the codesandbox with all three different variations. https://codesandbox.io/s/red-https-bxpo0?file=/src/App.tsx Basically, boils down to using a Map inside the state object. Starting from a state shape that looks like and an acton that adds an item into an array, if...
I can update the PR with the test that checks this, but I'm not sure if you want something thatcould be fixed in user land here? |
ssorallen left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the example of mutating theMap differently happens to get around the bug that is caused by passing a new reducer touseReducer on each render. The bug still exists, although I can't track down exactly why the "CorrectMap" example works.
The reducer not being memoized is definitely causing a double dispatch for me too, it's a side effect of passing a different function touseReducer on each render.
This PR should includeeffectReducer in the dependency list, and that actually surfaces an issue in the Codesandbox: the reducer function passed touseEffectReducer must either be a module-level variable or beuseMemo'd itself otherwise each render creates a new reducer function and gets back into this bug.
Updated Codesandbox with the reducer moved to a module function and witheffectReducer added to the deps array:https://codesandbox.io/s/elated-williams-fzbmg
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Ross Allen <ssorallen@users.noreply.github.com>
chrisdhanaraj commentedApr 26, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@davidkpiano - hello! I know it's been a while, but checking to see if there was something I could do to get this merged. I swapped to this version in an internal codebase and one thing that probably should be called out is the need to memoize the effectsMap object in userland as well. i.e., now that the |
Resolves#18
Essentially matches the
useMemo/useCallbackusage that the other wrapped functions (initialStateAndEffects / wrappedDispatch) already have