-
Notifications
You must be signed in to change notification settings - Fork 467
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 |
timdeschryver
left a comment
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_advanceTimersWrappernow receives anasyncfunction. 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
asyncunstable_advanceTimersWrapperWhy:
To support experimentation with
asyncactHow:
awaitadvanceTimersWrapper. This also enables us to flush the microtask queue that exist after the macrotask.This changes the observed semantics in
callbacka bit sincecallbacknow 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:
checkCallbacksemantics were never documented so this is fine.unstable_advanceTimersWrapperhas no docs either and I don't feel comfortable adding some.[ ]TypeScript definitions updated