esm: improve error messages for ambiguous module syntax #60376
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
| const undefinedGlobal = getUndefinedCJSGlobalLike(e?.message); | ||
| if (e?.name === 'ReferenceError' && undefinedGlobal !== null) { |
There was a problem hiding this comment.
| const undefinedGlobal = getUndefinedCJSGlobalLike(e?.message); | |
| if (e?.name === 'ReferenceError' && undefinedGlobal !== null) { | |
| const notDefinedGlobalLike = e?.name === 'ReferenceError' && findCommonJSGlobalLikeNotDefinedError(e.message); | |
| if (notDefinedGlobalLike) { |
There was a problem hiding this comment.
wdyt of this suggestion? That would remove one optional chaining, and skip the function call for unrelated errorrs
There was a problem hiding this comment.
Just my codding style. 😢 updated to use short-circuit evaluation. Thanks!
| '--input-type=module', | ||
| '--eval', | ||
| `await 1;\nconst fs = require('fs');`, | ||
| `const fs = require('fs');\nawait 1;`, |
There was a problem hiding this comment.
I changed the order to match the pattern in the existing test at line 278 (const fs = require("node:fs"); await Promise.resolve();). However, I realize this might be an unnecessary change if the original order was intentional. Should I revert this?
There was a problem hiding this comment.
Yeah I think it should be reverted, both order should be supported so it makes sense to test both IMO
There was a problem hiding this comment.
I've revert changes.
| const { stderr, code, signal } = await spawnPromisified( | ||
| process.execPath, | ||
| [ | ||
| '--input-type=module', |
There was a problem hiding this comment.
I removed --input-type=module because I saw that line 278 already tests the same error without the flag:
const fs = require("node:fs"); await Promise.resolve();I thought having both tests (with and without --input-type=module) was redundant since they produce the same error message. However, if there's a specific reason to test the explicit module type specification scenario separately, I can revert this change.
There was a problem hiding this comment.
I've revert changes.
There was a problem hiding this comment.
Maybe you have and you forgot to push your changes?
There was a problem hiding this comment.
It's already there. I had removed it in an earlier commit, but reverted it back in 9457af9.
- Refactor getUndefinedCJSGlobalLike to use ArrayPrototypeFind - Change if-else chain to switch statement - Add single quotes around global names in error messages - Revert test file to use --input-type=module flag and original order - Update test regex patterns to expect quoted global names
| ArrayPrototypeFind( | ||
| CJSGlobalLike, | ||
| (globalLike) => errorMessage === `${globalLike} is not defined`, | ||
| ) ?? null; |
There was a problem hiding this comment.
That's not useful, it it?
| ) ?? null; | |
| ); |
There was a problem hiding this comment.
Right.. It's redundant since ArrayPrototypeFind returns undefined when not found.
Thanks for review. I applied it
|
It looks like there are some conflicts here, could you please rebase your changes on top of the latest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #60376 +/- ##
=======================================
Coverage 88.53% 88.53%
=======================================
Files 703 703
Lines 208222 208239 +17
Branches 40142 40139 -3
=======================================
+ Hits 184357 184374 +17
- Misses 15852 15857 +5
+ Partials 8013 8008 -5
🚀 New features to boost your workflow:
|
Remove unused ArrayPrototypeSome import and fix assert usage.
Skip function call for non-ReferenceErrors and reduce optional chaining.
6 participants
Fixes: #60322
Improve ERR_AMBIGUOUS_MODULE_SYNTAX error messages to show the actual undefined global instead of always mentioning require().