module: improve error message for top-level await in CommonJS #55874

mertcanaltin · 2024-11-15T20:44:24Z

Added a specific error message for using top-level await in CommonJS modules. The error now suggests using ESM or wrapping await in an async function for clarity.

Fixes: #55776

codecov · 2024-11-15T22:05:54Z

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (968e2f4) to head (811e9c5).
Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	71.42%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55874      +/-   ##
==========================================
- Coverage   90.21%   90.21%   -0.01%     
==========================================
  Files         635      635              
  Lines      187580   187460     -120     
  Branches    36853    36831      -22     
==========================================
- Hits       169231   169112     -119     
- Misses      11108    11146      +38     
+ Partials     7241     7202      -39

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	`97.25% <100.00%> (+0.03%)`	⬆️
src/module_wrap.h	`0.00% <ø> (ø)`
src/module_wrap.cc	`72.54% <71.42%> (+0.10%)`	⬆️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

GeoffreyBooth

Thank you for tackling this! I’d love to see this issue addressed.

src/node_contextify.cc

GeoffreyBooth

Thanks for taking another pass at this. I mention this in the particular notes, but in general:

Please don't change lines based on personal preference or readability. We value the ability for git blame to point directly to the commit/PR that added or meaningfully changed a particular line, and that ability is diluted when unrelated PRs rewrite lines of code.
Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level await in CommonJS. Any other tests that fail as a result of your changes mean that there's a bug in the implementation that needs to be addressed. My guess is that the bug is that the new code isn't checking that the error message being thrown is specifically about top-level await. If there aren't currently tests for top-level await in CommonJS and your new error message, one or more should be added.

This is close; another pass and I think you may get it there. Thanks for your effort!

src/node_contextify.cc

test/es-module/test-esm-detect-ambiguous.mjs

mertcanaltin · 2024-11-30T20:40:18Z

Thanks for taking another pass at this. I mention this in the particular notes, but in general:

Please don't change lines based on personal preference or readability. We value the ability for git blame to point directly to the commit/PR that added or meaningfully changed a particular line, and that ability is diluted when unrelated PRs rewrite lines of code.

Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level await in CommonJS. Any other tests that fail as a result of your changes mean that there's a bug in the implementation that needs to be addressed. My guess is that the bug is that the new code isn't checking that the error message being thrown is specifically about top-level await. If there aren't currently tests for top-level await in CommonJS and your new error message, one or more should be added.

This is close; another pass and I think you may get it there. Thanks for your effort!

thank you very much for the review, I will be sending a new commit following what you said

mertcanaltin · 2024-12-02T18:28:41Z

Greetings, when I did as you suggested to cover only the top level error, I saw that all tests passed except 1 test

GeoffreyBooth

Thanks, this is much better. What’s the one test that is failing?

test/es-module/test-esm-detect-ambiguous.mjs

mertcanaltin · 2024-12-03T15:00:24Z

Thanks, this is much better. What’s the one test that is failing?

I saw wrong, all the tests passed my local. 🙏

GeoffreyBooth · 2024-12-03T21:07:41Z

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

cjihrig · 2024-12-03T21:21:36Z

why isn’t the CI output showing me which test is failing within that file?

No clue. But the output is incomplete. There is no summary at the end either. It looks like maybe the Python runner killed the Node process or otherwise truncated its output.

cjihrig · 2024-12-03T21:39:50Z

@GeoffreyBooth just FYI - IIRC, you asked me a similar question in Slack last year. There was output truncated only in GitHub Actions. Then you asked me to find a test run where truncation was happening without the use of node:test, and I did. I believe @MoLow was a part of that thread as well.

GeoffreyBooth · 2024-12-04T01:24:08Z

IIRC, you asked me a similar question in Slack last year.

Yes, last August: https://openjs-foundation.slack.com/archives/C019Y2T6STH/p1691860008875109. I’m lucky if I can remember last month, much less last year. Sorry to repeat. I also opened #49120 but that got closed.

mertcanaltin · 2024-12-04T09:17:45Z

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

yes, ı view my local enviorement this test is fail

test at test/es-module/test-esm-detect-ambiguous.mjs:367:5
✖ does not warn when there are no package.json (63.625667ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + "(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn't parse as CommonJS.\n" +
  +   'Reparsing as ES module because module syntax was detected. This incurs a performance overhead.\n' +
  +   'To eliminate this warning, add "type": "module" to /Users/mert/package.json.\n' +
  +   '(Use `node --trace-warnings ...` to show where the warning was created)\n'
  - ''
  
      at TestContext.<anonymous> (file:///Users/mert/Desktop/openSource/node/test/es-module/test-esm-detect-ambiguous.mjs:372:7)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:932:9)
      at async Promise.all (index 3)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async Promise.all (index 6)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:297:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn\'t parse as CommonJS.\nReparsing as ES module because module syntax was detected. This incurs a performance overhead.\nTo eliminate this warning, add "type": "module" to /Users/mert/package.json.\n(Use `node --trace-warnings ...` to show where the warning was created)\n',
    expected: '',
    operator: 'strictEqual'
  }

mertcanaltin · 2024-12-10T20:13:53Z

greetings I tried to make the error message more descriptive, in addition I tried to throw it when there is a file that cannot be successfully parsed as ESM

mertcanaltin · 2024-12-17T10:45:23Z

hello, I would be very grateful if you could look here when you are available, thank you very much @GeoffreyBooth

nodejs-github-bot · 2024-12-18T06:46:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64095/

nodejs-github-bot · 2024-12-18T18:23:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64104/

GeoffreyBooth

@mertcanaltin Thank you for your persistence. I think the approach of “a test is failing, so I’ll update the test” isn’t serving us well here; most of these updated tests shouldn’t need updating to achieve the goals of this PR, and the fact that they’re failing is an indicator that there are bugs in the implementation.

test/es-module/test-esm-detect-ambiguous.mjs

mertcanaltin · 2024-12-18T20:30:46Z

@mertcanaltin Thank you for your persistence. I think the approach of “a test is failing, so I’ll update the test” isn’t serving us well here; most of these updated tests shouldn’t need updating to achieve the goals of this PR, and the fact that they’re failing is an indicator that there are bugs in the implementation.

many thanks for your answers I will improve this place

src/node_contextify.cc

joyeecheung · 2025-01-20T18:12:59Z

I feel for the specific case mentioned in #55776, merely amending the error message is not good enough. It would've been clearer to either completely revert to the CJS exception (i.e. even the source line should be where the top-level await is at, not where require is at), or show both the exception that would be thrown if it's parsed as CJS, and the exception that would be thrown if it's parsed as ESM, to make it clear to the users that it cannot be parsed as neither, and print solution for each one as a warning (if they meant to use CJS, wrap the await in async function; if they meant to use ESM, do not use require).

mertcanaltin · 2025-05-31T19:31:30Z

I feel for the specific case mentioned in #55776, merely amending the error message is not good enough. It would've been clearer to either completely revert to the CJS exception (i.e. even the source line should be where the top-level await is at, not where require is at), or show both the exception that would be thrown if it's parsed as CJS, and the exception that would be thrown if it's parsed as ESM, to make it clear to the users that it cannot be parsed as neither, and print solution for each one as a warning (if they meant to use CJS, wrap the await in async function; if they meant to use ESM, do not use require).

Hello, now I tried to highlight both errors that can occur as you said

GeoffreyBooth · 2025-06-02T02:47:26Z

lib/internal/modules/esm/module_job.js

      isCommonJSGlobalLikeNotDefinedError(e.message)) {
+
+    if (e.message.includes('require is not defined')) {


Checking for require is not defined seems like it belongs within isCommonJSGlobalLikeNotDefinedError.

Yes, I've removed it, thank you.

GeoffreyBooth · 2025-06-02T02:48:31Z

lib/internal/modules/esm/module_job.js

+        'ERR_AMBIGUOUS_MODULE_SYNTAX: This file cannot be parsed as either CommonJS or ES Module.',
+        '- CommonJS error: await is only valid in async functions.',
+        '- ES Module error: require is not defined in ES module scope.',
+        'If you meant to use CommonJS, wrap top-level await in async function.',
+        'If you meant to use ESM, do not use require().',


This error message assumes that a top-level await is present, but simply checking for require is not defined wouldn’t necessarily imply that, would it?

You're absolutely right! My current implementation only checks for the "require is not defined" error but doesn't verify that top-level await is actually present. This could show misleading error messages for files that only use require() without any await.
I should add an additional check to detect top-level await in the source code before showing this specific error message.

GeoffreyBooth · 2025-06-02T02:49:56Z

lib/internal/modules/esm/module_job.js

+        '- ES Module error: require is not defined in ES module scope.',
+        'If you meant to use CommonJS, wrap top-level await in async function.',
+        'If you meant to use ESM, do not use require().',
+      ].join('\n');


Creating an array of strings and joining them is unnecessary processing to do at runtime. It would be more performant to just define a single string in the first place.

Also I can't think of other instances where we return multiline error messages. Unless this is a pattern we already use elsewhere, this should be rephrased to be one line. It doesn't need to be a single sentence, but it shouldn't include a newline.

I absolutely agree, I have updated this as a single line

GeoffreyBooth · 2025-06-02T02:50:53Z

test/es-module/test-esm-undefined-cjs-global-like-variables.js

@@ -38,5 +38,5 @@ assert.rejects(

 assert.rejects(
  import('data:text/javascript,require;//.js'),
-  /^(?!use the '\.cjs' file extension).*$/
+  /^(?:(?!use the '\.cjs' file extension)[\s\S])*$/


Why this change?

the regex here was broken at first so I updated it, but now the Asis version seems healthy, so I reverted it

When I looked here, I got an error for an unspelled period character in a regular expression, so I updated it
https://github.com/nodejs/node/actions/runs/15426765636/job/43415537965?pr=55874

mertcanaltin requested review from GeoffreyBooth and joyeecheung November 15, 2024 20:44

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Nov 15, 2024

GeoffreyBooth reviewed Nov 16, 2024

View reviewed changes

src/node_contextify.cc Outdated Show resolved Hide resolved

src/node_contextify.cc Outdated Show resolved Hide resolved

mertcanaltin added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Nov 27, 2024

mertcanaltin force-pushed the dev-55776 branch 3 times, most recently from 76f87e4 to c9531ef Compare November 29, 2024 21:21

mertcanaltin added review wanted PRs that need reviews. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Nov 30, 2024

mertcanaltin requested a review from GeoffreyBooth November 30, 2024 16:36

GeoffreyBooth reviewed Nov 30, 2024

View reviewed changes

mertcanaltin requested a review from GeoffreyBooth December 2, 2024 19:31

GeoffreyBooth reviewed Dec 2, 2024

View reviewed changes

test/es-module/test-esm-detect-ambiguous.mjs Outdated Show resolved Hide resolved

test/es-module/test-esm-detect-ambiguous.mjs Outdated Show resolved Hide resolved

GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Dec 2, 2024

mertcanaltin requested a review from GeoffreyBooth December 10, 2024 20:13

GeoffreyBooth reviewed Dec 18, 2024

View reviewed changes

joyeecheung reviewed Jan 20, 2025

View reviewed changes

src/node_contextify.cc Outdated Show resolved Hide resolved

mertcanaltin force-pushed the dev-55776 branch from 01b4b6d to a873a22 Compare April 18, 2025 18:12

mertcanaltin added 2 commits May 31, 2025 22:18

module: update tests for combined ambiguous module syntax error

77c3d19

lint

10e5620

mertcanaltin force-pushed the dev-55776 branch from a873a22 to 10e5620 Compare May 31, 2025 19:29

mertcanaltin requested review from GeoffreyBooth and joyeecheung May 31, 2025 19:31

GeoffreyBooth reviewed Jun 2, 2025

View reviewed changes

mertcanaltin added 10 commits June 2, 2025 23:38

revert regex edit

b0d51ac

repair error message

ab7ee6e

refactored ERR_AMBIGUOUS_MODULE_SYNTAX error

5725135

repair hasTopLevelAwait & tests

f9bd373

lint

aab10e5

revert old change

d756f7f

test repair

a2a9302

repair e.message

53b03da

register HasTopLevelAwait method in cpp api

32247d1

lint

811e9c5

		isCommonJSGlobalLikeNotDefinedError(e.message)) {

		if (e.message.includes('require is not defined')) {

Uh oh!

module: improve error message for top-level await in CommonJS #55874

Are you sure you want to change the base?

module: improve error message for top-level await in CommonJS #55874

Conversation

mertcanaltin commented Nov 15, 2024

Uh oh!

codecov bot commented Nov 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Nov 30, 2024

Uh oh!

mertcanaltin commented Dec 2, 2024

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 3, 2024

Uh oh!

GeoffreyBooth commented Dec 3, 2024

Uh oh!

cjihrig commented Dec 3, 2024

Uh oh!

cjihrig commented Dec 3, 2024

Uh oh!

GeoffreyBooth commented Dec 4, 2024

Uh oh!

mertcanaltin commented Dec 4, 2024

Uh oh!

mertcanaltin commented Dec 10, 2024

Uh oh!

mertcanaltin commented Dec 17, 2024

Uh oh!

nodejs-github-bot commented Dec 18, 2024

Uh oh!

nodejs-github-bot commented Dec 18, 2024

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mertcanaltin commented Dec 18, 2024

Uh oh!

Uh oh!

joyeecheung commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mertcanaltin commented May 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 15, 2024 •

edited

Loading

joyeecheung commented Jan 20, 2025 •

edited

Loading

mertcanaltin Jun 3, 2025 •

edited

Loading