module: throw error when re-runing errored module jobs #58957
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #58957 +/- ##
==========================================
+ Coverage 90.09% 90.10% +0.01%
==========================================
Files 640 640
Lines 188462 188452 -10
Branches 36962 36972 +10
==========================================
+ Hits 169786 169796 +10
+ Misses 11404 11376 -28
- Partials 7272 7280 +8
🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
request-ci
| }, { | ||
| message: 'test', | ||
| }); | ||
| })().catch(common.mustNotCall()); |
There was a problem hiding this comment.
We want to catch never-settling promises (EDIT: I've opened #58992 to enforce that at the linter level, so it's fine to land it as is, I can update the other PR once this one has landed)
| })().catch(common.mustNotCall()); | |
| })().then(common.mustCall()); |
Any reason not to use TLA/.mjs here?
There was a problem hiding this comment.
If the promise never settles the test should just fail with exit code 13. On the other hand using catch with must not call would surface unwanted rejections.
If we use .mjs then we need to use createRequire() - I'd rather keep the plain require() since that's the more commonly hit path if some boilerplate is inevitable.
There was a problem hiding this comment.
If the promise never settles the test should just fail with exit code 13
I think that's the case only for TLA, which we are not using here; you can convince yourself by adding a await new Promise(() => {}) at the top of the IIFE and see the test still passes.
|
|
||
| assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), { | ||
| message: 'test', | ||
| }).catch(common.mustNotCall()); |
There was a problem hiding this comment.
Same here
| }).catch(common.mustNotCall()); | |
| }).then(common.mustCall()); |
| await assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), { | ||
| message: 'test', | ||
| }); |
There was a problem hiding this comment.
nit: it would be interesting to validate we catch the exact value being thrown, not just one with the same message.
aduh95
added
author ready
nodejs-github-bot
removed
the
commit-queue
|
Landed in 793a279 |
Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error.
Drive-by: add some debug logs.
Fixes: #58945