useEffectReducer icon indicating copy to clipboard operation
useEffectReducer copied to clipboard

memoize the wrappedReducer function

Open chrisdhanaraj opened this issue 3 years ago • 3 comments

Resolves #18

Essentially matches the useMemo/useCallback usage that the other wrapped functions (initialStateAndEffects / wrappedDispatch) already have

chrisdhanaraj avatar Aug 19 '20 02:08 chrisdhanaraj

Thanks for this! Can you add a quick test to demonstrate the fixed behavior?

davidkpiano avatar Aug 19 '20 04:08 davidkpiano

Okay, so this is somewhat of a goof on me. My solve does 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

interface State {
  selectedItems: Map<string, string[]>
}

and an acton that adds an item into an array, if...

  1. When you handle the event in the reducer, if you operate on the original map in state, then create a new Map and use that in the returned state, you run into the double dispatch problem.

  2. But memoing the wrapped reducer in the useEffectReducer solves this, as I believe this keeps the identity the same. This matches how you would solve the same issue in useReducer (i.e., either memo the reducer function or move it out of the React component).

  3. But but you can also avoid the double dispatch problem by handling Maps better. Instead of operating on the original Map and then cloning as you updated state, you can instead clone the original Map then operate on it. If you do that, I believe you still maintain identity and the double dispatch doesn't happen.

I can update the PR with the test that checks this, but I'm not sure if you want something that could be fixed in user land here?

chrisdhanaraj avatar Aug 19 '20 06:08 chrisdhanaraj

@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 useMemo has the [effectsMap..] in it, users should be doing something like

const effectsMap = useMemo(() => {
 return {}
}, [whateverDepsRequired])

chrisdhanaraj avatar Apr 26 '21 21:04 chrisdhanaraj