-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: add async to onFinish report event #623
fix: add async to onFinish report event #623
Conversation
✔️ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: 34071b2 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/61ef39456cebba000791774c 😎 Browse the preview: https://deploy-preview-623--vitest-dev.netlify.app |
975c8e7
to
1ca66fe
Compare
Adding |
async behind a function transform the function to a awaitable. The function report which make sure to send report event needs function which return promise. |
Ok, makes sense that this will return a promise now. |
Hi, tested it on my repo that was crashing on |
Same here 👌 |
Good comment, I think we can remove the Awaitable type. |
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 think it's better to make them all Awaitable
to support async implementation in reporters. Also shouldn't they await Promise.all
and not just turn into an async function?
async function onFinished(files?: File[] | undefined) {
await Promise.all(
this.clients.map((client) => {
return client.onFinished?.(files)
})
)
}
Actually, events prefixed with vitest/packages/vitest/src/api/setup.ts Line 104 in c23be10
Should work |
Thanks for the PR!
|
I have made the fix in 6b02dea and released it as v0.2.3. Please check if it fixes the error |
@antfu I've tried The issue provides reproduction steps but it also has a screenshot of where the error is thrown - a Promise.reject is invoked but there's no try/catch for it in when using async/await, could it be that there's no error boundary in the callstack to catch it, so node stops the execution? |
I've now tested two local repo's that had the issue prior to V0.2.3 but after upgrading I'm no longer seeing the messages and/or crashes happening (with and without ui flag). Stackblitz seems to not complain anymore as well. |
@mitchelvanbever on a second look at the original image from @edimitchel, the error messages are indeed not the same, although they both show a similar warning + stacktrace (both caused by unhandled promise rejections). This one is related to custom reporters. Any ideas how unhandled promise rejections could be further prevented or at least caught, for the sake of a clearer stacktrace? |
@antfu what should we do ? |
I think it should be fixed now. Can you confirm? If so, we may close the PR |
Co-authored-by: webfansplz <>
Error when using UI with Vitest