fix(cleanup): remove scheduler code from flush-microtasks #744
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 6faf0bc:
|
src/__tests__/cleanup.js
Outdated
| @@ -31,7 +31,7 @@ test('cleanup waits for queued microtasks during unmount sequence', async () => | |||
| const spy = jest.fn() | |||
|
|
|||
| const Test = () => { | |||
| React.useEffect(() => () => setImmediate(spy)) | |||
| React.useEffect(() => () => setTimeout(spy, 200)) | |||
There was a problem hiding this comment.
Users can do all kinds of async things as a result of their cleanup functions and it's not reasonable for them to expect that we'll wait until all those async things are done. They'll have to wait for those things themselves.
So we shouldn't have a setImmediate in here at all.
| const spy = jest.fn() | ||
|
|
||
| const Test = () => { | ||
| React.useEffect(() => () => setImmediate(spy)) | ||
| React.useEffect(() => spy) |
There was a problem hiding this comment.
All we can guarantee is that the cleanup function is called, so that's all we'll test.
There was a problem hiding this comment.
👍 The problem is that we can't know what kind of task is queued from the cleanup function. We only supported setImmediate but any longer running task wasn't caught as far as I can tell.
Codecov Report
@@ Coverage Diff @@
## master #744 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 100 100
Branches 16 16
=========================================
Hits 100 100 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This one looks good and as @eps1lon said, we were probably only waiting for setImmediate anyways.
Looks like we got ourselves to the wrong adventure 😅 Sorry about that.
I'll close the PR for mocking shceduler (#743).
This PR will resolve https://github.com/testing-library/react-testing-library/issues/738 and #735.
|
Thanks for all the work you did anyway @MatanBobi! |
|
Glad I was able to help @kentcdodds |
|
🎉 This PR is included in version 10.4.6 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
@kentcdodds I think the react-native package suffers from a similar bug. When I add |
Hi @pke! |
What: This removes the scheduler code from flush-microtasks
Why: Because the test that failed is fundamentally flawed
How:
Checklist:
- [ ] Documentation added to the docs siteN/A- [ ] Typescript definitions updatedN/ACloses #743