Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

memoize the wrappedReducer function#19

Open
chrisdhanaraj wants to merge2 commits intodavidkpiano:master
base:master
Choose a base branch
Loading
fromchrisdhanaraj:feat/memo-reducer

Conversation

@chrisdhanaraj
Copy link

Resolves#18

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

ssorallen and aganoza reacted with thumbs up emoji
@davidkpiano
Copy link
Owner

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

@chrisdhanaraj
Copy link
Author

chrisdhanaraj commentedAug 19, 2020
edited
Loading

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

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 Ibelieve this keeps the identity the same. This matches how you would solve the same issue inuseReducer (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 andthen cloning as you updated state, you can instead clone the original Mapthen 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 thatcould be fixed in user land here?

Copy link

@ssorallenssorallen left a comment
edited
Loading

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

Co-authored-by: Ross Allen <ssorallen@users.noreply.github.com>
@chrisdhanaraj
Copy link
Author

chrisdhanaraj commentedApr 26, 2021
edited
Loading

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

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

1 more reviewer

@ssorallenssorallenssorallen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Non-memoized / ref'd reducer runs into double dispatch issue when used in custom hook

3 participants

@chrisdhanaraj@davidkpiano@ssorallen

[8]ページ先頭

©2009-2025 Movatter.jp