Skip to content

[fixed] Cancel requested animation frame on unmount. #883

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

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

adriantrunzo
Copy link
Contributor

@adriantrunzo adriantrunzo commented Jun 5, 2021

Fixes #882 .

Changes proposed:

  • Be sure to cancel the animation frame that is requested in the ModalPortal open method so that a state change is not potentially requested after the component is unmounted.

In the issue description I mentioned that isMounted might be a potential fix, but then I read that it is an antipattern. The componentWillUnmount method is already clearing a timeout, so canceling the animation seems like the best option.

I can confirm that these changes fix the error message I was seeing in the jest/testing-library environment I am using in a separate project. It is unclear to me how to reproduce this race condition in the testing setup used in this project. Any ideas would be appreciated.

Acceptance Checklist:

  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 84.474% when pulling ba8b6c2 on adriantrunzo:master into fc76b0c on reactjs:master.

@diasbruno diasbruno merged commit 0847049 into reactjs:master Jun 5, 2021
@diasbruno
Copy link
Collaborator

Good catch! Thanks.

@mbclu
Copy link

mbclu commented Jun 15, 2021

Is it possible to get this released? The project I'm working on is trying to pull in react-modal but finding that our test suite is leaving some warnings sprinkled about that I suspect this might resolve.

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
        in ModalPortal (created by Modal)
        in Modal (created by Modal)
        in Modal (created by ConfirmationModal)
        in ConfirmationModal (created by View)
        in View (created by LoadingContent)
        in div (created by LoadingContent)

@diasbruno
Copy link
Collaborator

Released version 3.14.3.

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.

Potential for memory leak via setState.
4 participants