module: fix error thrown from require(esm) hitting TLA repeatedly #55520

joyeecheung · 2024-10-24T12:55:47Z

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow.

Fixes: #55516
Refs: #52697

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow.

nodejs-github-bot · 2024-10-24T12:55:52Z

Review requested:

@nodejs/loaders
@nodejs/vm

codecov · 2024-10-24T14:18:06Z

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.43%. Comparing base (7b5d660) to head (ee2822b).
Report is 26 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/module_job.js	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55520      +/-   ##
==========================================
+ Coverage   88.41%   88.43%   +0.01%     
==========================================
  Files         653      654       +1     
  Lines      187476   187690     +214     
  Branches    36083    36126      +43     
==========================================
+ Hits       165763   165987     +224     
- Misses      14946    14949       +3     
+ Partials     6767     6754      -13

Files with missing lines	Coverage Δ
lib/internal/errors.js	`96.99% <100.00%> (+<0.01%)`	⬆️
lib/internal/modules/esm/loader.js	`98.02% <100.00%> (+<0.01%)`	⬆️
src/module_wrap.cc	`75.87% <100.00%> (-0.11%)`	⬇️
lib/internal/modules/esm/module_job.js	`99.25% <92.85%> (-0.75%)`	⬇️

... and 42 files with indirect coverage changes

nodejs-github-bot · 2024-10-26T22:47:20Z

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

nodejs-github-bot · 2024-10-28T12:33:52Z

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

nodejs-github-bot · 2024-10-28T15:00:25Z

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

test/es-module/test-require-module-tla-retry-require.js

src/module_wrap.cc

test/es-module/test-require-module-tla-retry-require.js

…peatedly

nodejs-github-bot · 2024-10-29T17:54:14Z

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

nodejs-github-bot · 2024-10-29T19:51:25Z

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

JakobJingleheimer · 2024-10-29T20:45:24Z

lib/internal/errors.js

@@ -1650,6 +1650,9 @@ E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
 E('ERR_QUIC_CONNECTION_FAILED', 'QUIC connection failed', Error);
 E('ERR_QUIC_ENDPOINT_CLOSED', 'QUIC endpoint closed: %s (%d)', Error);
 E('ERR_QUIC_OPEN_STREAM_FAILED', 'Failed to open QUIC stream', Error);
+E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' +
+  'graph with top-level await. Use import() instead. To see where the' +


I think referencing the graph is, whilst completely accurate, too esoteric. I expect many users won't even know what a graph is.

You mean the word "graph" in general or using "graph" to mean a module and its dependencies? I believe graph is a very common term used across different programming languages, including referring to the concept of "modules and its dependencies". There are also many module graph visualization tools out there, and its used extensively in materials such as MDN's guide on ESM and V8's user-facing explainer about ESM. If you Google "javascript module graph" you can find numerous tutorials and blog posts introducing ESM mentioning the concept of module graphs, and I suspect many learned about ESM by reading them so they should already know what this is.

nodejs-github-bot · 2024-10-29T21:15:21Z

Landed in 8aac7da

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: nodejs#55520 Fixes: nodejs#55516 Refs: nodejs#52697 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: #55520 Fixes: #55516 Refs: #52697 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: nodejs#55520 Fixes: nodejs#55516 Refs: nodejs#52697 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

This tracks the asynchronicity in the ModuleWraps when they turn out to contain TLA after instantiation, and throw the right error (ERR_REQUIRE_ASYNC_MODULE) when it's required again. It removes the freezing of ModuleWraps since it's not meaningful to freeze this when the rest of the module loader is mutable, and we can record the asynchronicity in the ModuleWrap right after compilation after we get a V8 upgrade that contains v8::Module::HasTopLevelAwait() instead of searching through the module graph repeatedly which can be slow. PR-URL: #55520 Fixes: #55516 Refs: #52697 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Jacob Smith <[email protected]>

joyeecheung mentioned this pull request Oct 24, 2024

Wrong ERR_REQUIRE_CYCLE_MODULE for require(esm) #55516

Closed

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2024

fixup! module: fix error thrown from require(esm) hitting TLA repeatedly

8e2df47

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2024

joyeecheung added the review wanted PRs that need reviews. label Oct 28, 2024

jsumners-nr reviewed Oct 29, 2024

View reviewed changes

test/es-module/test-require-module-tla-retry-require.js Outdated Show resolved Hide resolved

legendecas reviewed Oct 29, 2024

View reviewed changes

src/module_wrap.cc Show resolved Hide resolved

ShogunPanda approved these changes Oct 29, 2024

View reviewed changes

joyeecheung commented Oct 29, 2024

View reviewed changes

test/es-module/test-require-module-tla-retry-require.js Outdated Show resolved Hide resolved

fixup! Open module: fix error thrown from require(esm) hitting TLA re…

ee2822b

…peatedly

jsumners-nr approved these changes Oct 29, 2024

View reviewed changes

aduh95 approved these changes Oct 29, 2024

View reviewed changes

legendecas approved these changes Oct 29, 2024

View reviewed changes

joyeecheung added request-ci Add this label to start a Jenkins CI on a PR. and removed review wanted PRs that need reviews. labels Oct 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 29, 2024

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2024

JakobJingleheimer approved these changes Oct 29, 2024

View reviewed changes

joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 29, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 29, 2024

nodejs-github-bot merged commit 8aac7da into nodejs:main Oct 29, 2024
71 checks passed

aduh95 mentioned this pull request Nov 6, 2024

2024-11-11, Version 23.2.0 (Current) #55741

Merged

joyeecheung mentioned this pull request Nov 7, 2024

[v22.x] backport unflagging of --experimental-require-module and related fixes/refactorings #55217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: fix error thrown from require(esm) hitting TLA repeatedly #55520

module: fix error thrown from require(esm) hitting TLA repeatedly #55520

joyeecheung commented Oct 24, 2024 •

edited

Loading

nodejs-github-bot commented Oct 24, 2024

codecov bot commented Oct 24, 2024 •

edited

Loading

nodejs-github-bot commented Oct 26, 2024

nodejs-github-bot commented Oct 28, 2024

nodejs-github-bot commented Oct 28, 2024

nodejs-github-bot commented Oct 29, 2024

nodejs-github-bot commented Oct 29, 2024

JakobJingleheimer Oct 29, 2024

joyeecheung Oct 29, 2024 •

edited

Loading

nodejs-github-bot commented Oct 29, 2024

module: fix error thrown from require(esm) hitting TLA repeatedly #55520

module: fix error thrown from require(esm) hitting TLA repeatedly #55520

Conversation

joyeecheung commented Oct 24, 2024 • edited Loading

nodejs-github-bot commented Oct 24, 2024

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

nodejs-github-bot commented Oct 26, 2024

nodejs-github-bot commented Oct 28, 2024

nodejs-github-bot commented Oct 28, 2024

nodejs-github-bot commented Oct 29, 2024

nodejs-github-bot commented Oct 29, 2024

JakobJingleheimer Oct 29, 2024

Choose a reason for hiding this comment

joyeecheung Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 29, 2024

joyeecheung commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

joyeecheung Oct 29, 2024 •

edited

Loading