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(unmount): Flush useEffect cleanup functions syncronously #746

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jul 15, 2020

What:

Flush cleanup functions from useEffect synchronously on unmount.

Why:

react@next no longer flushes cleanup functions from useEffect synchronously.
Verified in https://travis-ci.org/github/testing-library/react-testing-library/builds/708314737

How:

Wrap unmountComponentAtNode in act.

Checklist:

  • [ ] Documentation added to the
    docs site
  • Tests
  • [ ] Typescript definitions updated
  • Ready to be merged

I'm not touching cleanup and flush-microtasks for now. We can do that in a follow-up PR that we test extensively against existing repos.

@eps1lon eps1lon added the bug Something isn't working label Jul 15, 2020
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 15, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3865525:

Sandbox Source
React Configuration
kentcdodds/react-testing-library-examples Configuration

@eps1lon eps1lon force-pushed the feat/unmount-cleanup-effect branch from ba59d1f to 6ef64ea Compare July 15, 2020 12:12
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #746 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #746   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          100       101    +1     
  Branches        16        16           
=========================================
+ Hits           100       101    +1     
Impacted Files Coverage Δ
src/pure.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 240900c...3865525. Read the comment docs.

@eps1lon eps1lon requested a review from kentcdodds July 15, 2020 12:37
@marcosvega91
Copy link
Member

marcosvega91 commented Jul 15, 2020

just a question, this should be done also here?

ReactDOM.unmountComponentAtNode(container)

EDIT
ok I read your note at the end of the PR

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks 👍

@kentcdodds kentcdodds merged commit b82773c into testing-library:master Jul 15, 2020
@eps1lon eps1lon deleted the feat/unmount-cleanup-effect branch July 15, 2020 12:50
@kentcdodds
Copy link
Member

🎉 This PR is included in version 10.4.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants