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

fix(hook): sequential updater calls operate on latest state #7

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

YellowKirby
Copy link
Contributor

Description

Hola! I found what I think is a 🐛 bug where multiple setState() calls from useMutative() both operate on the initial state, which can lead to unexpected dropped data. Here's a somewhat forced example:

const [state, setState] = useMutative({
  items: [1],
});

function someHandler() {
  setState((draft) => {
    draft.items.push(2);
  });

  setState((draft) => {
    // `draft.items` doesn't include the 2
    // that was pushed above :(
    draft.items.push(3);
  });
}

Once the next render happens, state.items would be [1, 3] (only the last call is applied), instead of the cumulative [1, 2, 3].

A fix

This PR changes how the hook's updateState() function is defined so that it uses the callback form of React setState so that each invocation of useMutative operates on the latest/up-to-date draft. Added a new test to verify the behavior :)

This also has a nice side-effect of making the updateState() callback identity stable, so it doesn't change between renders (although maybe it should when options changes?).

@unadlib
Copy link
Member

unadlib commented Apr 12, 2024

Thank you for the PR, I will merge this PR once the test passes.

@YellowKirby
Copy link
Contributor Author

If you have a moment, do you mind taking a look at the job failure? As far as I can tell, the problem might be because the PR branch is coming from a fork?

# tests seem okay here?
PASS test/index.test.ts
...

Bad response: 422 {"message":"Couldn't find a repository matching this job.","error":true}

I'm not entirely sure where to go from here.

@unadlib unadlib merged commit d6acbb6 into mutativejs:main Apr 12, 2024
1 check failed
@unadlib
Copy link
Member

unadlib commented Apr 12, 2024

hi @YellowKirby , thanks again for reporting the issue, I have also fixed the other issues.

The use-mutative v1.1.0 has been released; feel free to use it.

@YellowKirby YellowKirby deleted the multiple-updates branch April 12, 2024 20:09
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

Successfully merging this pull request may close these issues.

2 participants