-
Notifications
You must be signed in to change notification settings - Fork 471
feat(waitFor): Support async unstable_advanceTimersWrapper
#1229
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
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 c699137:
|
Codecov Report
@@ Coverage Diff @@
## main #1229 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 1042 1038 -4
Branches 351 347 -4
=========================================
- Hits 1042 1038 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expected a breaking change with this, but I tested this on a small project and everything continues to work fine 👍
FYI: This change breaks a lot of tests in my Vue project. I haven't taken the time to make a reproduction repo yet. Downgrading to 9.2.0 fixes the issue, or increasing the timeout on |
// of parallelization so we're fine. | ||
// https://stackoverflow.com/a/59243586/971592 | ||
// eslint-disable-next-line no-await-in-loop | ||
await advanceTimersWrapper(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious as an outsider, why is await
and async
needed here when we don't explicitly await
anything? Are we trying to wait a tick?
setTimeout(r, 0) | ||
jest.advanceTimersByTime(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if this was a bug but shouldn't the order of these 2 statement been reversed? ie. the promise should resolve after advancing timers?
unstable_advanceTimersWrapper
now receives anasync
function. If your wrapper just returns the given callback, nothing should break (e.g. this is not a breaking change for React Testing Library. But if you call it first with the expectation that it resolved synchronously, you may observe changed behavior. This method is marked asunstable_
and not subject to semantic versioning. Still releasing as a feature for safe measure.What:
Add support for having an
async
unstable_advanceTimersWrapper
Why:
To support experimentation with
async
act
How:
await
advanceTimersWrapper
. This also enables us to flush the microtask queue that exist after the macrotask.This changes the observed semantics in
callback
a bit sincecallback
now observes macrotask+microtask instead of microtask+macrotask.I don't think that matters though. We definitely have no test in this library and RTL tests passed using this version of
waitFor
.Checklist:
checkCallback
semantics were never documented so this is fine.unstable_advanceTimersWrapper
has no docs either and I don't feel comfortable adding some.[ ]TypeScript definitions updated