module: disable CommonJS support if entry point is ESM

[aduh95]

If the entry point is using ESM syntax and there's no package.json file to determine the default "type", disable CJS support and try to load the file again.
This is an attempt to help newcomers get started with Node.js without forcing them to use .mjs or have a package.json to use ESM syntax. It should be noted that using .mjs (or packge.json#type) would give you much better startup performance, I wouldn't recommend anyone to use this hack on production code.

Behavior on master:

(node:22414) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
…:1
console.log('hey it works');export{}
                            ^^^^^^

SyntaxError: Unexpected token 'export'
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1025:15)
    at Module._compile (node:internal/modules/cjs/loader:1059:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47

Behavior with this PR:

(node:22430) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
(node:22430) Warning: No package.json detected, attempting to load "…" with CommonJS support disabled.
hey it works

/cc @nodejs/modules

Fixes: #39353

[ljharb]

Instead of a hack, why not give a very clear error message? Teaching newcomers how to fix the problem will be more helpful to them in the long run than silently/magically pretending this is the way it’s supposed to work.

[aduh95]

It’s not silent: currently it shows a warning advising to use .mjs extension before displaying the SyntaxError stack trace. With this PR, instead of the SyntaxError, it shows another warning to tell that CommonJS is being disabled and they it loads the entry point as ESM. To the point of the reporter of the issue: if Node.JS suspects that the file is using ESM, why not try to parse it as that at this point?

[ljharb]

Because it can’t know that for sure - it’s a heuristic, because the parsing goals are ambiguous. node shouldn’t ever guess.

The modules group explicitly and intentionally avoided any chance of being incorrect about the parse goal.

[bmeck]

we should not have something that makes the interpretation of a file change depending on if it is loaded via the CLI or via import/require. This was discussed at length in the modules WG and in general the idea starts to cause more problems than it helps over time: from being a heuristic having issues with detection and problems with forwards compatibility, to having problems when run under wrappers like PM2/mocha/etc., to having a less clear at a glance or debuggable situation when trying to understand what a file will execute as. If we are to have such a change as this heuristic detection it should always be on to prevent these problems. However, I am unclear how we would do so. Perhaps an alternative is implementing something like the --package flag described in #38028 that would allow a stable interpretation of what format a file is within the process still and is intended for development/testing purposes per discussion. Designs like that allow for a beginner to still lack a package.json file but don't cause tech debt and allow things like wrapping processes to properly act the same as a CLI entry point. Still, heuristics are tricky and deserve their own discussion. Using them to enhance errors is one thing but I don't think we should be using them to alter runtime behavior.

[aduh95]

The modules group explicitly and intentionally avoided any chance of being incorrect about the parse goal.
we should not have something that makes the interpretation of a file change depending on if it is loaded via the CLI or via import/require.

I would argue this does not change the interpretation of a particular file, it disables CJS support altogether.
That shouldn't change anything for an external tool to determine the parse goal of that file – if it's using .js without a package.json, it's a CJS file (unless if there's no CJS support in said tool, like in Rollup).

problems with forwards compatibility

What if we add in the warning that this behavior is experimental and could be removed in a semver-patch release? Would that address this concern?

Perhaps an alternative is implementing something like the --package flag described in #38028 that would allow a stable interpretation of what format a file is within the process still and is intended for development/testing purposes per discussion.

I'm all for it, I still think this PR would make node more beginner-friendly.

Because it can’t know that for sure - it’s a heuristic, because the parsing goals are ambiguous. node shouldn’t ever guess.

I know that, I don't say this would cover 100% of the case. What it does is to check if the file contains any import or export declaration, and if there is any, chances are pretty high that it is indeed ESM syntax and that the user simply used the wrong extension, so it tries that.

Using them to enhance errors is one thing but I don't think we should be using them to alter runtime behavior.

Is there a way we could have this feature and you would feel confident users will not rely on it? Maybe by making the warning more annoying? By rethrowing the SyntaxError after executing the file maybe? Or do you think there is no way at all?

[ljharb]

@aduh95 CJS support should never be disabled in any possible scenario where node is running. (also, what about makeRequire in ESM? what would that do with a .js file without a package.json, which the entire node ecosystem at this point considers to be a CJS format?)

[bmeck]

I would argue this does not change the interpretation of a particular file, it disables CJS support altogether.
That shouldn't change anything for an external tool to determine the parse goal of that file – if it's using .js without a package.json, it's a CJS file (unless if there's no CJS support in said tool, like in Rollup).

The problem is similar still. I don't think stating that all support is altered makes any single file more stable with this feature.

What if we add in the warning that this behavior is experimental and could be removed in a semver-patch release? Would that address this concern?

That would not be addressed; various features of ESM have become more ambiguous with CJS over time (HTML comments, import(), etc.). Forwards compatibility is trying to narrow down things to stuff that wouldn't collide over time; unfortunately that is hard to quantify so avoiding ambiguity in the first place is an easy solution. With each patch to your module and/or moving code around it potentially is unstable with this heuristic. E.G. using import() only because of a top level await of some kind for bootstrapping would be CJS mode as an example of how forwards compatibility around ESM features is unsafe.

I'm all for it, I still think this PR would make node more beginner-friendly.

I think we should step back and talk about what this PR means for beginners rather than making that claim. I think we should think about what this does to people trying to begin using Node.js , not all of which are novice programmers:

1. If you lack a package.json, adding one alters this feature; and most node code bases do contain a package.json and/or in tutorials for node a package.json is created by tooling such as npm. The creation of that package.json would cause errors in this feature most likely since the default mode is for .js to be CJS. This would cause confusion most likely as this common tutorial step would break their codebase but likely lack insight into why it broke since .js working as CJS isn't something we warn on or really expect to be unexpected.

2. For people porting purely ESM code from non-node codebases that lack package.json this feature might work initially; however, there is potential it won't work from the beginning due to ambiguity like @ljharb points out.

3. For people writing code using this feature, having that code relying on disabling CJS be used as a dependency can cause problems. E.G.

const opts = parse(process.argv);
if (opts.command === 'a') {
  (await import('./esm-lib-a.js'))(opts)
} else if (opts.command === 'b') {
  (await import('./esm-lib-b.js'))(opts)
}

This would not disable CJS and so various files would likely be treated as CJS.

4. For people using this feature, many node code bases/libraries won't work since many contain CJS files.

I know that, I don't say this would cover 100% of the case. What it does is to check if the file contains any import or export declaration, and if there is any, chances are pretty high that it is indeed ESM syntax and that the user simply used the wrong extension, so it tries that.

I'm stating that it does have more effect than using the wrong extension for a file, it has affects with other features of node and isn't really a good fix compared to an error message and a user choosing the best solution for their issue. Using the wrong extension and completely disabling parts of core that aren't intending to be removed/deprecated are pretty wild to treat as an expected entanglement. I agree they likely wanted to run as ESM, what I don't agree with is the result of trying to guess how they want the module system to work or how we help them avoid the problem. I don't see this PR as helping over time; it might help initial setup in some cases, but often it looks like it will cause problems down the line. This is why having a flag or something would basically opt-in to some behavior temporarily rather than by assumption.

Is there a way we could have this feature and you would feel confident users will not rely on it? Maybe by making the warning more annoying? By rethrowing the SyntaxError after executing the file maybe? Or do you think there is no way at all?

I think asking how to make users feel that a feature is unreliable is a sign that maybe we should figure out how to make a feature reliable instead. I don't think I'm in general thinking this feature is reliable; questions about how to make users feel confident it shouldn't be used is a scary thing to be asked and I don't really want to approach it.

[weswigham]

This would make the interpretation of files for editors and tooling even more ambiguous than it already is - at least right now, the lack of a package file is a strong indicator of cjs. With this? Ehhhh.... We go to a world where a loose js file could be anything, which we very much wanted to avoid the ambiguity of. We cant reliably report IDE errors at that point.

[aduh95]

This would make the interpretation of files for editors and tooling even more ambiguous than it already is - at least right now, the lack of a package file is a strong indicator of cjs. With this? Ehhhh.... We go to a world where a loose js file could be anything, which we very much wanted to avoid the ambiguity of. We cant reliably report IDE errors at that point.

If an IDE has to handle a .js and there's no package.json in its scope, I don't think it's safe to assume it's a CJS module written for Node.js, it could be an ESM file written for browser usage (or Deno).

@aduh95 CJS support should never be disabled in any possible scenario where node is running.

FWIW It's already possible to do what this PR is doing using loaders. And I don't see why it would be the case, not everyone is using CJS.

also, what about makeRequire in ESM? what would that do with a .js file without a package.json, which the entire node ecosystem at this point considers to be a CJS format?

// entryPoint.js
import { createRequire } from 'node:module'
import { fileURLToPath } from 'node:url'
createRequire(import.meta.url)(fileURLToPath(import.meta.url));

Currently:

1. Node.js tries to load entryPoint.js as CJS
2. it fails with a SyntaxError
3. Node.js re-parses the file (or the error message) to check if it contains static import or export declarations.
4. If it does, it emits a warning To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
5. The SyntaxError stack trace is displayed to stderr and the process terminates.

With this PR:
5. Node.js changes the ESM loader to treat CJS files as modules.
6. It tries to parse and execute entryPoint.js as ESM, and emit another warning to say it's doing something risky.
7. The files tries to require itself, and because it's a .js file without a package.json in scope, it is treated as CJS.
8. The parsing fails with SyntaxError: Cannot use import statement outside a module and the process exits.

In the end, users still see a SyntaxError saying they're outside of a module, so I don't think it's any worse than the current implementation. (In the dummy example I've written above, it's the exact same error message that's being displayed). I suppose we could try to make createRequire throw when under this mode to make sure CJS is out of the way.

1. lack insight into why it broke since .js working as CJS isn't something we warn on or really expect to be unexpected.

I'm not sure that's fair, there would be two warnings displayed each time they run their app with node:

Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
Warning: No package.json detected, attempting to load "…" with CommonJS support disabled.

The creation of that package.json would cause errors in this feature most likely since the default mode is for .js to be CJS.
This would not disable CJS and so various files would likely be treated as CJS.

And they would be back at the current situation, which is not great IMHO, but seems to be the best we are able to make 🤷‍♂️
At least, it would have helped them familiarize with JS and Node.js built-in modules without forcing them to learn what CJS is, and hopefully it translates the burden of understanding how to enable ESM to a (ever so slightly) more experienced dev. I don't have any data but anecdotal evidence to back up this claim (that this feature would help beginners) though, so if no one backs up the proposal in the modules team or the community, I'll close the PR no hard feelings.

[ljharb]

They should be forced to learn what CJS is - it’s unavoidable. Most of npm is, and forever will be, CJS.

[guybedford]

I always thought of "type": "module" in the package.json like an HTML doctype - something new users will just do without understanding why they need to do it. Although interestingly it turns out there's also a new wave of web developers with no idea what a doctype is and leaving it out :P

Parsing was discussed at length yes. Performance was sighted as a concern. We now run cjs-module-lexer over all CommonJS files though, since that was able to get around the performance problems with a low-overhead lexer and Wasm. In theory this same lexer could be doing ESM syntax detection providing a reliable way to determine if a given JS file definitely has esm syntax. That argument is new to this old discussion, and I'm open to extending cjs-module-lexer to permit this type of syntax detection, but that doesn't change the other arguments here that there is still the surprise that removing an export changes the fact that your source code is running in strict mode or not, which was always the ambiguity concern. Wesley's argument about IDE compatibility also is an important one re predictable patterns for tooling assistance which only grows more important each day.

Happy to dig in if anyones opinions have changed on the above though. I really would have hoped by now that npm init and related tooling would flag a missing "type": "module" or package.json and ensure users are set up correctly.

[ljharb]

(the npm rfc calls decided npm init should not include type module because the type flag is confusing and problematic, and best left out - not a debate for this thread tho)

[guybedford]

the npm rfc calls decided npm init should not include type module because the type flag is confusing and problematic

That seems an unfortunate outcome. Do you have a link to more information here? We can give them more time sure, but at the end of the day tooling needs to solve tooling problems.

[bmeck]

In the end, users still see a SyntaxError saying they're outside of a module, so I don't think it's any worse than the current implementation. (In the dummy example I've written above, it's the exact same error message that's being displayed). I suppose we could try to make createRequire throw when under this mode to make sure CJS is out of the way.

This doesn't solve all of the other problems in my comment above and does have them fix the warning. It also changes from the entry point being seen as ESM, to instead thinking all files are ESM; which to me is non-intuitive.

I'm not sure that's fair, there would be two warnings displayed each time they run their app with node:

To be clear I'm talking about when that warning disappears because cases that generate package.json files like npm install . After that file is generated, there isn't really clarity on what broke (files were seen as ESM, but now are seen as CJS).

And they would be back at the current situation, which is not great IMHO, but seems to be the best we are able to make 🤷‍♂️
At least, it would have helped them familiarize with JS and Node.js built-in modules without forcing them to learn what CJS is, and hopefully it translates the burden of understanding how to enable ESM to a (ever so slightly) more experienced dev. I don't have any data but anecdotal evidence to back up this claim (that this feature would help beginners) though, so if no one backs up the proposal in the modules team or the community, I'll close the PR no hard feelings.

Only being able to use node builtins is... not very useful when most of the tutorials on node out there use things in the ecosystem like React or Express. Those do not currently ship ESM implementations.

[bmeck]

Also, to be clear using .mjs is easier for tutorials currently and is guaranteed to act like a Module, hence why the HTML standard now uses it to distinguish Module code examples. I think we should step back and ask about how much this simplifies in actuality vs how it acts on a glance.

[aduh95]

Closing as stalled.