-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
workers: make Worker async disposable #58385
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
base: main
Are you sure you want to change the base?
Conversation
jasnell
commented
May 19, 2025
```js await using worker = new Worker(...); ```
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58385 +/- ##
==========================================
+ Coverage 90.23% 90.24% +0.01%
==========================================
Files 633 633
Lines 186883 186888 +5
Branches 36695 36688 -7
==========================================
+ Hits 168632 168656 +24
+ Misses 11040 11039 -1
+ Partials 7211 7193 -18
🚀 New features to boost your workflow:
|
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.
LGTM with or without the nit; and s/^workers/worker/
in the commit message.
Noting that the code change can be safely backported to release lines that have Symbol.asyncDispose
but not the ERM itself; and splitting this into two commits (one that adds [SymbolAsyncDispose]()
, tests it by calling explicitly, and documents without code example; and another for v24+ adding using
in tests in docs) might make it more straightforward for releasers.
let w; | ||
{ | ||
// Verifies that the worker is async disposable | ||
await using worker = new Worker('for(;;) {}', { eval: true }); | ||
w = worker; | ||
w.on('exit', common.mustCall()); | ||
} |
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.
Nit: was w
meant to be checked after the scope? If not, we don't really need it.
let w; | |
{ | |
// Verifies that the worker is async disposable | |
await using worker = new Worker('for(;;) {}', { eval: true }); | |
w = worker; | |
w.on('exit', common.mustCall()); | |
} | |
{ | |
// Verifies that the worker is async disposable | |
await using worker = new Worker('for(;;) {}', { eval: true }); | |
worker.on('exit', common.mustCall()); | |
} |
Why
dont-land-on-v22.x
|
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.
lgtm