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

Add defensive isMounted checks before setting state via useMutation Hook #9883

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

alessbell
Copy link
Member

@alessbell alessbell commented Jul 5, 2022

This PR adds checks in two places inside of useMutation to verify that the component is mounted before setting state with setResult, which fixes #9858.

Testing

Spying on console.error here is sufficient. I've updated the tests, which now fail if either isMounted check added in the PR is removed.

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@dylanwulf
Copy link
Contributor

It looks like I'm at an impasse when it comes to making assertions against component state after the component unmounts (I want to call unmount in the middle of my test—it's also called automatically after each test via cleanup function—in order to ensure the state hasn't changed), as subsequent calls to waitForNextUpdate time out. Would love any suggestions

What I did in #9801 was just spy on console.error and assert that it never got called. Probably not the best solution, but it works!

@alessbell
Copy link
Member Author

What I did in #9801 was just spy on console.error and assert that it never got called. Probably not the best solution, but it works!

For this case, that should suffice :) Updated - thanks!

@alessbell alessbell force-pushed the issue-9858-useMutation-state-update-after-unmount branch from fbdd0f4 to 7ddaeeb Compare July 6, 2022 15:38
@alessbell
Copy link
Member Author

@benjamn removing either isMounted check this PR adds now fails the test (thanks to @dylanwulf's suggestion 👍). Updated and squashed into a single commit.

@alessbell alessbell requested a review from benjamn July 6, 2022 15:40
@alessbell alessbell changed the title [Issue #9858] useMutation state update after unmount Add defensive isMounted checks before setting state via useMutation Hook Jul 7, 2022
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo - this is really great @alessbell (and thanks for the spying on console.error suggestion @dylanwulf 🙇). A big 👍 from me!

@alessbell alessbell merged commit 43a7703 into main Jul 13, 2022
@alessbell alessbell deleted the issue-9858-useMutation-state-update-after-unmount branch July 13, 2022 23:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useMutation encounters error "Can't perform a React state update on an unmounted component"
3 participants