Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: remove --experimental-default-type #56092

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

GeoffreyBooth
Copy link
Member

This PR removes --experimental-default-type per the discussion and consensus at the November 2024 Dublin collaborator summit. This also follows the discussion in nodejs/TSC#1445 and related issues where it was decided that Node.js would not be flipping the default module system anytime soon, if ever, now that the use case of “run ESM syntax without needing to opt in” has been satisfied by module syntax detection. cc @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/test_runner

@nodejs-github-bot 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 Nov 30, 2024
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth removed the needs-ci PRs that need a full CI run. label Nov 30, 2024
Copy link

codecov bot commented Nov 30, 2024

Codecov Report

Attention: Patch coverage is 92.15686% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.00%. Comparing base (4ee87b8) to head (2e91784).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/esm/get_format.js 87.09% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #56092   +/-   ##
=======================================
  Coverage   88.00%   88.00%           
=======================================
  Files         656      656           
  Lines      189000   188927   -73     
  Branches    35995    35961   -34     
=======================================
- Hits       166320   166265   -55     
+ Misses      15840    15827   -13     
+ Partials     6840     6835    -5     
Files with missing lines Coverage Δ
lib/internal/main/check_syntax.js 100.00% <100.00%> (+1.26%) ⬆️
lib/internal/main/eval_stdin.js 100.00% <100.00%> (ø)
lib/internal/main/eval_string.js 79.66% <100.00%> (-0.34%) ⬇️
lib/internal/main/run_main_module.js 100.00% <100.00%> (ø)
lib/internal/modules/esm/formats.js 98.55% <100.00%> (ø)
lib/internal/modules/esm/load.js 90.12% <100.00%> (-0.17%) ⬇️
lib/internal/modules/run_main.js 97.60% <100.00%> (+0.29%) ⬆️
lib/internal/process/execution.js 98.79% <100.00%> (ø)
src/node_options.cc 87.87% <ø> (+0.32%) ⬆️
src/node_options.h 98.29% <ø> (ø)
... and 1 more

... and 23 files with indirect coverage changes

@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand this option is deemed unnecessary for now, from the notes I was unable to determine the underlying reasons.

Specifically, has it been decided that Node.js should never implement a flag to change the top-level interpretation?

And if not, why should we remove this now, and what would have to change in future, to reconsider such a flag?

Anyone coming back to this issue will probably want to know the answers to these questions, so it would be very useful to clearly state for reference.

@GeoffreyBooth
Copy link
Member Author

Specifically, has it been decided that Node.js should never implement a flag to change the top-level interpretation?

No formal decision has been made; landing this PR would be effectively making the decision, but only until someone lands a PR to add the flag back. Decisions are never final. If you'd like more discussion at the TSC level perhaps we could add this PR to the TSC agenda and you could join the next TSC meeting?

We know that flipping the default module system would be a breaking change to at least some (probably large) percentage of users, even years from now. I think the main question is: how would ESM-by-default be meaningfully different than the current status quo of syntax detection enabled by default, that would justify the disruption?

I can't really think of any differences that users would notice, other than ambiguous ESM JavaScript loading without a warning to add the type field; but that doesn't feel worth the disruption to me. The other use case would be running extensionless Wasm without a type field, but I'm unaware of that being something that anyone is doing; and we can support that without flipping the defaults if we really want to. Extensionless Wasm should currently throw when used as the entry point without --experimental-default-type=module, so it wouldn't be a breaking change to start running it.

And if not, why should we remove this now, and what would have to change in future, to reconsider such a flag?

The reasons to remove this are:

  1. It adds a meaningful amount of maintenance burden to the modules codebase; 29 files changed and 800 lines of code, per this PR.

  2. It also adds a lot of potential user confusion to the modules docs; there are many places where we need to mention that the described behavior is inverted if --experimental-default-type=module is passed.

  3. Syntax detection satisfies the same core use cases as --experimental-default-type, of being able to run ESM syntax without needing to opt-in somehow, without being a breaking change. And importantly, syntax detection only works in one direction: we can try to run as CommonJS first and retry as ESM on parse error, but trying to run as ESM first wouldn't generate a parse error for typical CommonJS code. (Something like node --input-type=module --eval 'const fs = require("fs")' throws a ReferenceError, a runtime error, rather than the ParseErrors that we look for in syntax detection. We can't use runtime errors in syntax detection because then we're not just parsing code twice, but potentially running it twice.) So I think that flipping the default probably means also getting rid of syntax detection, and therefore abandoning the ability to run either module system's code without needing an opt-in somehow. Extensionless JavaScript files would only work when written as ESM, for example, not as either ESM or CommonJS as they do now.

As for what would have to change in the future, in the various discussions (sorry, I can't find links) there was talk about potentially revisiting this and flipping the default several years from now, once the transition to ESM is truly far along and not just writing but also running CommonJS syntax is anachronistic; but per https://github.com/wooorm/npm-esm-vs-cjs#data we're still at something like two-thirds of most popular npm libraries being CommonJS, with movement of only about 4% in the past year. At this rate it will be many years before we pass the tipping point, though hopefully require(esm) will accelerate things a bit. We can always put the flag back in when that occurs, along with a plan for actually going through with the flip. I don't think we should keep the flag around for the next five years or so in the meantime, as another experimental zombie that's neither removed nor moving toward stability. We already have too many of those.

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🎉

Copy link
Member

@JakobJingleheimer JakobJingleheimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day, maybe :)

@aduh95 aduh95 added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 1, 2024
@GeoffreyBooth GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit 94c327c into nodejs:main Dec 2, 2024
80 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 94c327c

@ljharb ljharb deleted the remove-default-type branch December 2, 2024 22:45
targos pushed a commit that referenced this pull request Dec 3, 2024
PR-URL: #56092
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
@joyeecheung
Copy link
Member

Belated LGTM but I agree with #56092 (comment) - hopefully with require(esm) bridging migration the ESM tipping point will happen earlier than WW3, we'll see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants