module: support ESM detection in the CJS loader #52047

joyeecheung · 2024-03-11T19:45:33Z

module: support ESM detection in the CJS loader

This patch:

Adds ESM syntax detection to compileFunctionForCJSLoader()
for --experimental-detect-module and allow it to emit the
warning for how to load ESM when it's used to parse ESM as
CJS but detection is not enabled.
Moves the ESM detection of --experimental-detect-module for
the entrypoint from executeUserEntryPoint() into
Module.prototype._compile() and handle it directly in the
CJS loader so that the errors thrown during compilation and
execution during the loading of the entrypoint does not
need to be bubbled all the way up. If the entrypoint doesn't
parse as CJS, and detection is enabled, the CJS loader will
re-load the entrypoint as ESM on the spot asynchronously using
runEntryPointWithESMLoader() and cascadedLoader.import(). This
is fine for the entrypoint because unlike require(ESM) we don't
the namespace of the entrypoint synchronously, and can just
ignore the returned value. In this case process.mainModule is
reset to undefined as they are not available for ESM entrypoints.
Supports --experimental-detect-module for require(esm)

nodejs-github-bot · 2024-03-11T19:45:39Z

Review requested:

@nodejs/loaders
@nodejs/startup
@nodejs/vm

GeoffreyBooth · 2024-03-11T20:17:25Z

My note from #51977 (comment) still applies; I’m not sure why we need a separate PR for this. If we support detection at all, it should be via one flag, not two; having separate flags for detection for import and detection for require is a footgun for users because they should never be enabling one without the other.

joyeecheung · 2024-03-13T14:13:29Z

Pasting my reply from #51977 (comment) :

I don't want to couple require(esm) with --experimental-detect-module because I think the implementation of the latter has a lot of issues that we talked about before (reading and parsing the file twice etc.) and it seems unfortunate to me that once you enable require(esm) detection you also get yourself into those troubles. But to fix --experimental-detect-module there are a lot to be done and would unnecessarily complicates require(esm), so might as well just keep detection out of the scope of this PR. I don't think it needs to cover all these to land as a first iteration experiment. Without detection this is already can load all the ~20 high impact esm-only npm packages that I've tested just fine. Detection only serves a less painful point that can be already be addressed with explicit module type while there's no workaround in core for require(esm) at all ATM. IMO the latter is the major pain point of ESM adoption while the former is minor and less of a priority, so I just want to focus on the higher priority.

I don't think require(esm) must support every feature there is (experimental ones, even) to land as an experiment itself. That would just procrastinate the entire thing. My criteria for whether the first iteration is feature complete enough is very simple - can require(esm) load say the top 20 high-impact ESM-only npm packages? If yes, then it's good enough as a MVP. We can sort out other stuff while it's experimental. If we want to enforce that experimental features must work with other features to land, then either SEA shouldn't land because it doesn't support import, or --experimental-detect-module shouldn't land because it doesn't support SEA, for example.

GeoffreyBooth · 2024-03-13T23:29:13Z

I don’t think require(esm) must support every feature there is (experimental ones, even) to land as an experiment itself.

That’s fine; as I wrote over there, I don’t think the initial require(esm) needs to support detection in order to land. But this PR is just adding detection to require(esm), so I think it’s fair to say that this PR needs to support detection in a way that we agree on.

I also think detection can be part of the first PR, as it’s simple and all I requested was that you change the new flag you’re adding to instead use the existing flag. But if you’d prefer to do two PRs that’s fine too.

joyeecheung · 2024-03-14T01:47:27Z

I think it’s fair to say that this PR needs to support detection in a way that we agree on.

I agree but I think if this would need a lot more than just what this PR already has to fix --experimental-detect-module. That's why I opened a separate PR to avoid derailing #51977 I personally don't think ContainsModuleSyntax should be implemented the way it is and we've talked about it (i.e. we should rely on V8 to cache successful compilations, reparse it as ESM on syntax failure potentially using V8 API instead of error message matching, etc.). But I would not expect to just fix that with a few lines, and it feels really out of scope of require(esm). For now I just want to prioritize require(esm) and set aside other things in the ESM loader that I think look off.

GeoffreyBooth · 2024-03-14T03:37:16Z

I personally don’t think ContainsModuleSyntax should be implemented the way it is

Sure, but isn’t that a concern for unflagging it? Like those are just performance optimizations; you could add CommonJS detection to it and keep all detection behind the one flag, and then unflag everything once detection is refactoring to your liking.

joyeecheung · 2024-03-14T13:00:38Z

Sure, but isn’t that a concern for unflagging it? Like those are just performance optimizations; you could add CommonJS detection to it and keep all detection behind the one flag, and then unflag everything once detection is refactoring to your liking.

I think the detection in CJS should be done in where CJS compiles code - that's where it can be done most efficiently. If done that way the refactoring isn't really a separate thing and can cause constant conflicts.

joyeecheung · 2024-04-07T22:03:09Z

This needs #52413 to detect the ESM syntax properly in C++

joyeecheung · 2024-04-22T17:59:59Z

doc/api/modules.md

 3. If X.json is a file, load X.json to a JavaScript Object. STOP
 4. If X.node is a file, load X.node as binary addon. STOP
-5. If X.mjs is a file, and `--experimental-require-module` is enabled,


I noticed that this is actually unnecessary, when X is already a file, it's covered by 1 (and even before we supported require(esm), 1 already implied that X.mjs can be loaded as a ECMAScript module by require(), kind of funny..

This patch: 1. Adds ESM syntax detection to compileFunctionForCJSLoader() for --experimental-detect-module and allow it to emit the warning for how to load ESM when it's used to parse ESM as CJS but detection is not enabled. 2. Moves the ESM detection of --experimental-detect-module for the entrypoint from executeUserEntryPoint() into Module.prototype._compile() and handle it directly in the CJS loader so that the errors thrown during compilation *and execution* during the loading of the entrypoint does not need to be bubbled all the way up. If the entrypoint doesn't parse as CJS, and detection is enabled, the CJS loader will re-load the entrypoint as ESM on the spot asynchronously using runEntryPointWithESMLoader() and cascadedLoader.import(). This is fine for the entrypoint because unlike require(ESM) we don't the namespace of the entrypoint synchronously, and can just ignore the returned value. In this case process.mainModule is reset to undefined as they are not available for ESM entrypoints. 3. Supports --experimental-detect-module for require(esm).

.eslintignore

doc/api/modules.md

src/node_contextify.cc

nodejs-github-bot · 2024-04-22T18:12:19Z

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

src/node_contextify.cc

nodejs-github-bot · 2024-04-27T05:03:18Z

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

aduh95

The GHA CI is failing though

aduh95 · 2024-04-27T08:20:24Z

lib/internal/modules/esm/translators.js

+    assert(!isMain);
+    CJSModule._load(filename, null, isMain);


If isMain is guaranteed to be falsy, is there any reason to pass it to CJSModule._load?

Is there a reason not to pass it explicitly and rely on the implicit default value of CJSModule._load even though the value is readily available?

richardlau · 2024-04-28T14:03:14Z

https://github.com/nodejs/node/actions/runs/8850127576/job/24303670039?pr=52047#step:5:4444

../src/node_contextify.cc: In function ‘void node::contextify::CompileFunctionForCJSLoader(const v8::FunctionCallbackInfo<v8::Value>&)’:
../src/node_contextify.cc:1510:8: error: unused variable ‘is_main’ [-Werror=unused-variable]
 1510 |   bool is_main = args[2].As<Boolean>()->Value();
      |        ^~~~~~~

github-actions · 2024-04-29T10:17:21Z

Failed to start CI

   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/8876920917

nodejs-github-bot · 2024-04-29T10:18:52Z

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

joyeecheung · 2024-04-29T10:18:52Z

I kept forgetting the request-ci label is unusable now :(

nodejs-github-bot · 2024-04-29T13:48:30Z

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

nodejs-github-bot · 2024-04-29T20:21:55Z

Landed in 4d59a9d

This patch: 1. Adds ESM syntax detection to compileFunctionForCJSLoader() for --experimental-detect-module and allow it to emit the warning for how to load ESM when it's used to parse ESM as CJS but detection is not enabled. 2. Moves the ESM detection of --experimental-detect-module for the entrypoint from executeUserEntryPoint() into Module.prototype._compile() and handle it directly in the CJS loader so that the errors thrown during compilation *and execution* during the loading of the entrypoint does not need to be bubbled all the way up. If the entrypoint doesn't parse as CJS, and detection is enabled, the CJS loader will re-load the entrypoint as ESM on the spot asynchronously using runEntryPointWithESMLoader() and cascadedLoader.import(). This is fine for the entrypoint because unlike require(ESM) we don't the namespace of the entrypoint synchronously, and can just ignore the returned value. In this case process.mainModule is reset to undefined as they are not available for ESM entrypoints. 3. Supports --experimental-detect-module for require(esm). PR-URL: #52047 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This patch: 1. Adds ESM syntax detection to compileFunctionForCJSLoader() for --experimental-detect-module and allow it to emit the warning for how to load ESM when it's used to parse ESM as CJS but detection is not enabled. 2. Moves the ESM detection of --experimental-detect-module for the entrypoint from executeUserEntryPoint() into Module.prototype._compile() and handle it directly in the CJS loader so that the errors thrown during compilation *and execution* during the loading of the entrypoint does not need to be bubbled all the way up. If the entrypoint doesn't parse as CJS, and detection is enabled, the CJS loader will re-load the entrypoint as ESM on the spot asynchronously using runEntryPointWithESMLoader() and cascadedLoader.import(). This is fine for the entrypoint because unlike require(ESM) we don't the namespace of the entrypoint synchronously, and can just ignore the returned value. In this case process.mainModule is reset to undefined as they are not available for ESM entrypoints. 3. Supports --experimental-detect-module for require(esm). PR-URL: nodejs#52047 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

joyeecheung marked this pull request as draft March 11, 2024 19:45

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 11, 2024

joyeecheung mentioned this pull request Apr 7, 2024

module: detect ESM syntax by trying to recompile as SourceTextModule #52413

Merged

avivkeller added module Issues and PRs related to the module subsystem. cli Issues and PRs related to the Node.js command line interface. labels Apr 21, 2024

joyeecheung force-pushed the require-module-with-detection branch from 289a1cd to e559c26 Compare April 22, 2024 17:58

joyeecheung changed the title ~~[WIP] module: implement --experimental-require-module-with-detection~~ module: support ESM detection in the CJS loader Apr 22, 2024

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

joyeecheung marked this pull request as ready for review April 22, 2024 17:58

joyeecheung commented Apr 22, 2024

View reviewed changes

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

This comment was marked as outdated.

Sign in to view

joyeecheung force-pushed the require-module-with-detection branch from e559c26 to dac7020 Compare April 22, 2024 18:05

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

GeoffreyBooth reviewed Apr 22, 2024

View reviewed changes

.eslintignore Outdated Show resolved Hide resolved

doc/api/modules.md Outdated Show resolved Hide resolved

src/node_contextify.cc Show resolved Hide resolved

src/node_contextify.cc Outdated Show resolved Hide resolved

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

GeoffreyBooth added the esm Issues and PRs related to the ECMAScript Modules implementation. label Apr 22, 2024

joyeecheung mentioned this pull request Apr 25, 2024

Tracking issue: require(esm) #52697

Open

9 tasks

fixup! module: support ESM detection in the CJS loader

4645f8b

dygabo reviewed Apr 27, 2024

View reviewed changes

src/node_contextify.cc Show resolved Hide resolved

aduh95 approved these changes Apr 27, 2024

View reviewed changes

fixup! fixup! module: support ESM detection in the CJS loader

ff13fa7

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

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

github-actions bot added the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 29, 2024

GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 29, 2024

GeoffreyBooth approved these changes Apr 29, 2024

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 29, 2024

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

nodejs-github-bot merged commit 4d59a9d into nodejs:main Apr 29, 2024
63 checks passed

aduh95 mentioned this pull request Apr 30, 2024

2024-05-02, Version 22.1.0 (Current) #52768

Merged

marco-ippolito added the backport-blocked-v20.x PRs that should land on the v20.x-staging branch but are blocked by another PR's pending backport. label May 21, 2024

joyeecheung mentioned this pull request Aug 27, 2024

ERR_INTERNAL_ASSERTION thrown when using --experimental-require-module and --import together #54577

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: support ESM detection in the CJS loader #52047

module: support ESM detection in the CJS loader #52047

joyeecheung commented Mar 11, 2024 •

edited

Loading

nodejs-github-bot commented Mar 11, 2024

GeoffreyBooth commented Mar 11, 2024 •

edited

Loading

joyeecheung commented Mar 13, 2024 •

edited

Loading

GeoffreyBooth commented Mar 13, 2024

joyeecheung commented Mar 14, 2024 •

edited

Loading

GeoffreyBooth commented Mar 14, 2024 •

edited

Loading

joyeecheung commented Mar 14, 2024 •

edited

Loading

joyeecheung commented Apr 7, 2024

joyeecheung Apr 22, 2024

This comment was marked as outdated.

nodejs-github-bot commented Apr 22, 2024

nodejs-github-bot commented Apr 27, 2024

aduh95 left a comment

aduh95 Apr 27, 2024

joyeecheung Apr 27, 2024

richardlau commented Apr 28, 2024

github-actions bot commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

joyeecheung commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

module: support ESM detection in the CJS loader #52047

module: support ESM detection in the CJS loader #52047

Conversation

joyeecheung commented Mar 11, 2024 • edited Loading

nodejs-github-bot commented Mar 11, 2024

GeoffreyBooth commented Mar 11, 2024 • edited Loading

joyeecheung commented Mar 13, 2024 • edited Loading

GeoffreyBooth commented Mar 13, 2024

joyeecheung commented Mar 14, 2024 • edited Loading

GeoffreyBooth commented Mar 14, 2024 • edited Loading

joyeecheung commented Mar 14, 2024 • edited Loading

joyeecheung commented Apr 7, 2024

joyeecheung Apr 22, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Apr 22, 2024

nodejs-github-bot commented Apr 27, 2024

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Apr 27, 2024

Choose a reason for hiding this comment

joyeecheung Apr 27, 2024

Choose a reason for hiding this comment

richardlau commented Apr 28, 2024

github-actions bot commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

joyeecheung commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

nodejs-github-bot commented Apr 29, 2024

joyeecheung commented Mar 11, 2024 •

edited

Loading

GeoffreyBooth commented Mar 11, 2024 •

edited

Loading

joyeecheung commented Mar 13, 2024 •

edited

Loading

joyeecheung commented Mar 14, 2024 •

edited

Loading

GeoffreyBooth commented Mar 14, 2024 •

edited

Loading

joyeecheung commented Mar 14, 2024 •

edited

Loading