-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(cleanup): Cleanup should flush microtask queue after unmounting containers #632
fix(cleanup): Cleanup should flush microtask queue after unmounting containers #632
Conversation
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 5048091:
|
Hmm, CI seems to be stalled/not reporting? Should I push a non-change again? 🤔 |
Thanks for this @kamranayub! I'm conflicted about this one. I think it's good, but I'm worried that there could be unintended consequences of people missing things their components are doing that should be handled in their tests... Hmmm... I think I'm good to merge this, but I'd love someone else to take a look at this as well. |
I agree, my hope today was to make a CodeSandbox that showcased this more clearly with either react-query and react-modal, to see if the root cause could be fixed in underlying libraries. Part of me also thinks it could be a real use case, as you may expect microtask queue to work like this in a real usage context even on unmount. But I don't know enough to say with certainty. |
Codecov Report
@@ Coverage Diff @@
## master #632 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 4 +1
Lines 103 141 +38
Branches 15 23 +8
=========================================
+ Hits 103 141 +38
Continue to review full report at Codecov.
|
I have narrowed it down with a reproduction repo and isolated case of using |
@kentcdodds Good news, I seem to have been able to reproduce my issue solely with |
I was able to fix the underlying issue in Closing for now! 👍 |
Ok, so here's some fun news. I believe we actually do need this change. Check this out: const React = require('react')
const { render } = require('@testing-library/react')
function Comp() {
const [state, setState] = React.useState()
React.useEffect(() => {
setTimeout(() => setState('blah'))
}, [state])
return null
}
test('runs', () => {
render(React.createElement(Comp))
}) You'll notice if you run this, it passes without issue or warning. But it shouldn't! What should be happening here is |
Whoops, I think I didn't finish that thought and accidentally commented 😅 Honestly, I'm not sure what to do here and I'd love feedback. Please see: https://youtu.be/h7B3Ouv0gSE |
Actually, I'm pretty sure we should leave things as-is. Flushing first will mean that useEffect callbacks will get called if needed which will queue up cleanup callbacks which will then get called by the unmounting process. If we swap this, we could miss calling cleanups (or calling stale cleanups). |
I think we've figured this all out now. I haven't seen an act warning in a while :) |
jk, this literally popped up again and I'm sick of it. We're merging this thing. |
🎉 This PR is included in version 10.4.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
FWIW, this fixed my issues and I'm pretty sure it's not causing more harm than good so that's good news :) |
@all-contributors please add @kamranayub for code and tests |
I've put up a pull request to add @kamranayub! 🎉 |
In case anyone stumbles on this like I did where after this patch update, ALL your tests are broken -- except for the first one -- that runs in the suite, and the error is related to using the https://github.com/davidtheclark/focus-trap-react package with its
|
What:
The
cleanup
routine should unmount mounted containers first to trigger all the effect cleanup routines and then flush the microtask queue.Why:
When using some libraries such as
react-query
orreact-modal
, there can be unmounting routines that schedule immediate microtasks to perform cleanup.I've documented a specific case here in react-query where I was seeing testing inconsistencies unless I explicitly called
unmount
during the test.In React Modal specifically, here's a snippet from one of their
componentWillUnmount
cleanup routines:https://github.com/reactjs/react-modal/blob/v3.11.2/src/components/Modal.js#L153-L172
How:
I moved the
await flush()
to be after unmounting containers. I've added a test case that fails when usingsetImmediate
during an effect cleanup routine.Checklist:
docs site N/A
I think this is a safe (and correct) change but it is a change in behavior that could have side effects.