module: improve error message from asynchronicity in require(esm) #57126

joyeecheung · 2025-02-18T17:20:11Z

Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time.
Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available.

Drive-by: split the require(tla) tests since we are modifying the tests already.

Refs: https://github.com/fisker/prettier-issue-17139

nodejs-github-bot · 2025-02-18T17:20:17Z

Review requested:

@nodejs/loaders
@nodejs/vm

joyeecheung · 2025-02-18T17:22:06Z

lib/internal/modules/esm/loader.js

+      const parentFilename = urlToFilename(parent?.filename);
+      // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
+      if (!job.module) {
+        let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;


I can hit this branch via https://github.com/fisker/prettier-issue-17139 though I don't think it's reliably testable because it comes down to a race. From prettier/prettier#17139 it seems tiny changes to the module graph can make the race disappear. So it's probably not worth adding to the test suite in case this becomes a flaky test.

nodejs-github-bot · 2025-02-18T18:08:14Z

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

codecov · 2025-02-18T18:40:06Z

Codecov Report

Attention: Patch coverage is 79.62963% with 11 lines in your changes missing coverage. Please review.

Project coverage is 89.03%. Comparing base (2086877) to head (f2d314b).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/loader.js	57.14%	9 Missing ⚠️
src/node_errors.h	85.71%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57126      +/-   ##
==========================================
+ Coverage   89.02%   89.03%   +0.01%     
==========================================
  Files         665      665              
  Lines      193408   193451      +43     
  Branches    37283    37284       +1     
==========================================
+ Hits       172185   172243      +58     
- Misses      13882    13897      +15     
+ Partials     7341     7311      -30

Files with missing lines	Coverage Δ
lib/internal/errors.js	`97.47% <100.00%> (+0.01%)`	⬆️
lib/internal/modules/esm/module_job.js	`99.26% <100.00%> (+<0.01%)`	⬆️
src/module_wrap.cc	`75.76% <100.00%> (ø)`
src/node_errors.h	`85.29% <85.71%> (+0.29%)`	⬆️
lib/internal/modules/esm/loader.js	`97.01% <57.14%> (-0.86%)`	⬇️

... and 38 files with indirect coverage changes

If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally.

- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already.

joyeecheung · 2025-02-18T21:38:43Z

Looks like the THROW_ERR_* and ERR_* macros are not prepared for a single string message that might contain characters that would otherwise need escaping if used as the format string. Added a special case in the macros to deal with it (I suspect this is not the only place that can e.g. pass a path into the error message formatter which might contain %, though we might have been getting away with it if the test cases did not happen to test on those errors...)

nodejs-github-bot · 2025-02-18T23:58:48Z

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

nodejs-github-bot · 2025-02-20T00:21:05Z

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

nodejs-github-bot · 2025-02-20T17:22:10Z

Landed in 305d15a...8d10bc7

If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: #57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: #57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: nodejs#57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: nodejs#57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>