esm: detect ESM syntax in ambiguous JavaScript

[GeoffreyBooth]

Implements #50064. This PR aims to allow Node.js to allow ES module syntax input by default, without the user needing to opt in by using a non-.js extension, a package.json "type" field or a command-line flag. The goal is to make this enabled by default, unflagged, once we consider it stable.

For the following “ambiguous” inputs:

• Files with a .js extension or no extension; and either no controlling package.json or one that lacks a type field; and --experimental-default-type is not specified
• String input (--eval or STDIN) when neither --input-type nor --experimental-default-type are specified

Node will do the following:

• For ambiguous main entry point, either file or string input, the new containsModuleSyntax function from esm: improve check for ESM syntax #50127 is used to determine whether the CommonJS or ESM loader should handle the main entry. (The ESM loader is used if the main entry contains module syntax.)
• When the ESM loader is handling modules, within the load hook, once the source is loaded it will be passed to containsModuleSyntax to determine whether the format should be module or commonjs (again, only for the ambiguous inputs specified above).

This behavior applies to ambiguous files within node_modules. This is to prevent the hazard where a package author writes a library relying on this detection behavior but consumers of the package would fail to load it if they didn’t also have the detection behavior. (Such a hazard will still exist for consumers running old versions of Node, but at least it’s avoided moving forward.)

This behavior applies to the main entry point and to files referenced via import. It would not apply to files referenced via require, because require cannot load ES modules.

This behavior does not apply to modules with any marker of explicitness: an .mjs or .cjs extension, a package.json with a type field, specified --input-type or --experimental-default-type flags.

This behavior does not apply to --print input, as --print does not support ESM syntax.

When enabled, detection should have no impact on all-CommonJS apps (either written that way or transpiled into CommonJS) since detection isn’t run on files that are referenced via require; such apps would incur only a single double parse for the entry point (and therefore never opt into the ESM loader). The largest impact would be for ESM apps with CommonJS dependencies that lack type fields.

Before unflagging this we need to confirm that when enabled, this doesn’t affect any apps that run without erroring today. Or inversely it should have no effect on anything that already runs successfully, and therefore is not a breaking change.

cc @nodejs/performance @nodejs/loaders @nodejs/tsc

Notable change

The new flag --experimental-detect-module can be used to automatically run ES modules when their syntax can be detected. For “ambiguous” files, which are .js or extensionless files with no package.json with a type field, Node.js will parse the file to detect ES module syntax; if found, it will run the file as an ES module, otherwise it will run the file as a CommonJS module. The same applies to string input via --eval or STDIN.

We hope to make detection enabled by default in a future version of Node.js. Detection increases startup time, so we encourage everyone—especially package authors—to add a type field to package.json, even for the default "type": "commonjs". The presence of a type field, or explicit extensions such as .mjs or .cjs, will opt out of detection.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

There are tests failing because of the change in this PR where I move the attempted CommonJS module load out of the ModuleWrap callback to just before that callback is defined, because I need it earlier so that we can retry as ESM. (https://github.com/nodejs/node/pull/50096/files#diff-b1de5e9ce1e9b411001c63d11a5092dbba32e1586269a20dc11408375eb2f16cR309-R318.) The failing tests are ones that compare error messages and stacks, and I suspect that some of these are legitimate errors because of some special handling that happens to errors thrown within the ModuleWrap callback. If someone who knows C++ and/or ModuleWrap can help me figure out how to preserve the prior error handling behavior, I could use the assistance.

[mcollina]

I think @jasnell did some analysis on related topics using perf_hooks.

[GeoffreyBooth]

Looking at the first broken test, it’s for the debugger pausing on caught exceptions. The PR as currently written puts the “module load” within a try/catch, where inside the catch I check if we should do the “try again as ESM” logic, else rethrow the error. This new try/catch changes the error that the “debugger catches caught exceptions” test is asserting against.

To fix the test, I can move the “should we retry” condition outside of the try/catch, so it’s like “if we’re in detect/retry mode, try/catch and retry; else just module load.” But the issue there is that the test will pass now while there’s a flag, but once the flag is removed and we’re in detect/retry mode by default, I’ll be back to the broken test so long as the test case remains ambiguous code.

Which leads to the bigger question, is that should Node adding a try/catch around module loading be considered a breaking change? If not, then I can just update the test accordingly and move on. I have a feeling that many of the other broken tests will be things like this, where the tests are checking very specific things in Node’s internals (like particular stack traces, or async resources created) that change in this branch, even for CommonJS code that executes successfully. We can always mark this as semver-major, and go into detail in the release notes of the edge cases that are affected; most real-world code should run unaffected. Or if the things that change as a result of this are internals that we don’t consider part of the public API, then the changes shouldn’t be considered breaking.

[targos]

I'd like to see some tests that verify we don't catch runtime exceptions. For example, this should throw in CommonJS and not trigger the ESM re-evaluation.

eval('import "something";');

[GeoffreyBooth]

I’m going to redo this to use #50127 once that lands.

[GeoffreyBooth]

I’ve updated this to build on #50127. You can see the differences that this PR adds at https://github.com/GeoffreyBooth/node/compare/better-esm-check…GeoffreyBooth:node:ambiguous-detection

[karlhorky]

Thanks for this @GeoffreyBooth and everyone else involved! 🙌 Amazing to see these pieces being developed enabling the move to ESM default 👍

[karlhorky]

If anyone wants to play with this without installing Node.js v21.1.0 on your machine, I created a sandbox here that demonstrates the detection of the entry point:

CodeSandbox Demo: https://codesandbox.io/p/sandbox/restless-leaf-rhrxhp?file=%2F.devcontainer%2FDockerfile%3A2%2C1

[privatenumber]

Love this feature!

I attempted to leverage this in tsx to replace the CommonJS loader. Unfortunately, I wasn't successful because this feature requires strict ESM (no references to require, __filename, etc.), and some outdated packages may use a mix of ESM/CJS.

This behavior and constraint makes sense to me, especially for Node—just wanted to share a limitation I observed.

[wraithgar]

What do we do with all of the packages that exist today? They're idempotent and can't change.

I would humbly suggest this is a mistake. If I were to pick the number one lesson learned in the last 14 years of npm's history it would be: "Do not try to guess user intent". You have a clear way for folks to indicate module type, and over a decade of history for what that type is if this new feature is not being indicated. Why are we slowing down every package that exists for an implementation of guessing what the user wants?

[GeoffreyBooth]

@wraithgar That was considered; see the referenced issues. There are use cases that just can't be solved, such as ESM syntax in loose extensionless files, without either a breaking change or detection. Adding detection is the least bad option. We spent months considering other options.

It's been a strong recommendation for years in the Node docs that every package author include the type field. Once this feature becomes enabled by default, such packages will still work, they'll just increase the overall app startup time slightly. Runtime performance is unaffected. We felt that this is an acceptable trade-off for finally supporting the remaining use cases and to support ESM syntax by default without requiring an opt in.

If you'd like to discuss further please open a new issue.

[ljharb]

I've proposed a solution multiple times (including during the modules WG discussions) that doesn't require either one of those, but you keep ignoring it. An extensions map would be a superior nonbreaking replacement for type that would cover extensionless files.

[andrewbranch]

This will have some unfortunate implications for TypeScript if it becomes enabled by default in the future. I wrote up an explanation on our issue tracker: microsoft/TypeScript#56678