Proposal: Detect ESM syntax in every ambiguous file

[GeoffreyBooth]

Building off of #50043 (comment), this is an alternate proposal to #50043. This aims to achieve the same goals of allowing ESM syntax in “loose” files, where there are no package.json files present; and in files whose nearest parent package.json lacks a type field. This would permit ESM syntax without needing to opt in, while avoiding breaking existing scripts and tutorials. Like #50043, this proposal would avoid any breaking changes, and the aim is to eventually make this enabled by default without requiring a flag.

The new behavior would be as follows:

• Node would attempt to evaluate every ambiguous file (a file with no explicit .mjs or .cjs extension, either no package.json or one that lacks a type field) as CommonJS, as it already does today. If a file throws a SyntaxError for something that is only allowed in ES modules (import or export statement, import.meta, top-level await), Node would try again to evaluate the file as an ES module.
• This behavior would apply to ambiguous files within node_modules.
• This would apply to the main entry point and to files referenced via import. It would not apply to files referenced via require.
• This behavior would be disabled by any marker of explicitness: .mjs or .cjs extension, a package.json type field, --input-type or --experimental-default-type.

Pros, in comparison with #50043:

• Avoids the application behaving differently based on entry point: you can reference the entry point from a wrapper, such as a CommonJS test runner, and it will still run as ESM.
• Prevents the “package author published ambiguous ESM” problem. If a package author develops locally in this mode, relying on it to enable ESM for ambiguous files, their package would still work for consumers.
• This “per file” approach is the same as that taken by many other tools.

Cons:

• Presumably this is more of a performance impact, as we’re parsing every ambiguous ES module twice. We should probably measure this to see how much of an impact it is. There should be no impact for CommonJS files, and also no impact for ES modules in an explicit package.json module scope.
• Any files with ambiguous syntax, that could evaluate successfully as either CommonJS or ESM, would be run as CommonJS in this mode. A common example of such a file would be a polyfill that only sets a global variable, without importing or exporting anything.

@nodejs/loaders @nodejs/tsc

[bmeck]

I'd like to add a Pro:

• This "double parse" behavior is present in many tools while the alternative is not and generally would not add configuration burden to users of those tools or the tools themselves

Per general discussion:

I'd note if this is considered an error recovery mechanism rather than discrete choice of the module author swapping the default type to be ESM is likely not an issue as it would still fall into the error category. This would also make it sensible to have the ability to warn upon encountering this and allow fixing in an iterative manner.

If this is framed as an explicit design choice for userland authors rather than a recovery, it would be something authors see as reliable and likely would never be getting warnings about. I'd generally like to avoid this framing to allow a swap over and/or removing the ambiguity being seen as a good thing.

I would also note parsing files is DX cheaper than restarting a process and forcing updates to the code before doing so.

Performance impacts are likely something that could also be left to package authors. Generally, authors do like their library to be more performant. Additionally this impact is only once, during startup. The errors from import would generally be near the top of the file bailing out the parse early as well. The impact is further potentially lessened if we can reuse code cache between the parses.

[GeoffreyBooth]

Something we would need to decide is whether we’d want to print a warning on double parse. Something like “Node.js tried to run ./file.js as CommonJS but couldn’t, so it tried again to run it as an ES module. Avoid the performance penalty of Node.js making two attempts by adding "type": "module" to ./package.json.”

I think we wouldn’t want to warn on dependencies, as it would be annoying to see warnings triggered by third-party code. We could use the “under node_modules“ logic to decide not to show the warning. We also shouldn’t warn for files run with no controlling package.json file, as I think a double parse for loose files is an acceptable performance penalty.

The benefit here is that the user is made aware that they’re running their code more slowly than it needs to be run, and how to fix it. The downside is that this is another bit of friction for new users; it makes it seem like they’re doing something wrong (which in a sense, they are) which means that any ESM-first beginner tutorials would need to address this. Also is Node really “ESM-first” if it prints a warning if you don’t opt into ESM.

We should also do a benchmark to see just how much of a penalty this is. If it’s trivial, or if we can find a way to make it trivial, maybe it’s not worth warning about.

[bmeck]

I don't think the need to warn in a user visible way by default is there since this is opt-in. I'd think only if they wanted all/extra warnings they would see this?

[GeoffreyBooth]

since this is opt-in

The goal is to eventually enable this by default without a flag.

[bmeck]

@GeoffreyBooth then it should be mentioned in the proposal since having things be opt out is different from opt-in design wise for breakage.

[mcollina]

I think this proposal would slow things down for the average user, as well as printing a warning for everybody, as we all have quite old dependencies that are unlikely to get an update.

[GeoffreyBooth]

I think this proposal would slow things down for the average user, as well as printing a warning for everybody, as we all have quite old dependencies that are unlikely to get an update.

The warning isn’t part of the proposal per se; we don’t necessarily need to print any warnings at all, for anyone. It was just an idea, of something that we might additionally want to consider. I wouldn’t want any warning that was triggered by dependencies; it would need to somehow be scoped to user code.

As for performance, we don’t know until we test. By definition this new mode would do detection only for files that currently error, so no app that works today would get any slower. The question is how slow this “ESM by detection” mode is, compared against the baseline of “explicit ESM” (i.e. "type": "module"). Arguably any level of performance is better than erroring, so by that logic any amount of performance hit doesn’t matter; but as a matter of principle Node should be performant by default, without someone needing to know a secret to squeeze out the optimal performance. I think cjs-module-lexer is a good point of reference, where we have a Wasm dependency doing a parsing pass for every imported CommonJS module, and it’s so fast that it’s not a noticeable performance cost. If per-file detection can happen at that level of efficiency, it might be worth it.

One last thing about performance: module loading time generally only affects initial startup time, for the typical app that isn’t doing dynamic import()s long after initialization. Many users don’t care all that much how long their apps take to start up; if you’re building a web server, that’s intended to run for hours or days, it doesn’t matter all that much if it takes 1 second or 2 seconds to start. I think this is why so few Node users bother to bundle their apps into a single file or with minified code like is common for frontend; for most use cases, startup time just doesn’t matter. And for users who care, besides adding "type" they can also bundle. This isn’t to say that we shouldn’t care about startup time, but just that it’s perhaps not a disqualifying metric.

[bmeck]

quite old dependencies that are unlikely to get an update.

These should remain unaffected by this specific proposal since this only affects a well known error condition and doesn't propagate results from one file to others.

[joyeecheung]

I think cjs-module-lexer is a good point of reference, where we have a Wasm dependency doing a parsing pass for every imported CommonJS module

I don't think we need a WASM module for for the proposed use case, V8 should be fast enough because we are looking for early SyntaxErrors, instead of a particular legal way of property assignment. I doubt a WASM dependency can be faster than V8 in this regard, the V8 parser (or, to be precise, the V8 preparser that's used for compiling top-level code) is already built for looking for early syntax errors quickly. It's also likely that these files would have an import statement very early in the file, so V8 should fail quickly instead of wasting time on parsing the rest of the file.

[Qard]

I think having slightly slow ambiguous ESM is better than no ambiguous ESM if it's a fallback like this which should not impact CJS performance unless it fails with a SyntaxError. It gives us a path to be able to drop CJS someday in the future, if that possibility ever comes. (I personally have my doubts we will ever be able to drop CJS, but I see no harm in setting ourselves up to be able to if it ever becomes a reasonable possibility.)

[WebReflection]

I expect most of the time the error to be super early as import syntax is usually on top, not sure how much of a concrete slow down it is. I also would expect no warnings unless a flag is set … when such flag is set I’d expect all warnings including node_module folder entries, effectively allowing anyone to actually check possible ambiguous dependencies and either ignore these or deal with these.

This has the same goal of informing users which is not clear if they would ever benefit from such warning for their own entry point of choice, they could benefit as well for their dependency tree, imho.

Other than this, I’d love to see this happening ❤️

[bnoordhuis]

This would apply to the main entry point and to files referenced via import. It would not apply to files referenced via require.

Why is that? (I think I know but I'd like to see it explained explicitly.)