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

Start Node ESM stable version at Node16 #48879

Merged
merged 12 commits into from
May 3, 2022
Merged

Start Node ESM stable version at Node16 #48879

merged 12 commits into from
May 3, 2022

Conversation

DanielRosenwasser
Copy link
Member

Today Node 16 provides two really nice characteristics:

  • It supports all the resolution modes of Node 12 and Node 14 (a difference which won't be fixed)
  • It supports ES2022 features, including top-level await, ??=, #private fields, numeric separators, error causes, and more. This is nice because even if you're not transforming your input code, you won't get an error for using some of these constructs.

This PR starts our minimum stable version of Node targeting and resolution to Node 16 instead of Node 12.

There is a caveat which @weswigham pointed out, which is that top-level await isn't yet documented as stable.

Fixes #48646

@ghost
Copy link

ghost commented Apr 28, 2022

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ DanielRosenwasser sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@@ -33101,10 +33101,11 @@ namespace ts {
Diagnostics.await_expressions_are_only_allowed_at_the_top_level_of_a_file_when_that_file_is_a_module_but_this_file_has_no_imports_or_exports_Consider_adding_an_empty_export_to_make_this_file_a_module);
diagnostics.add(diagnostic);
}
if ((moduleKind !== ModuleKind.ES2022 && moduleKind !== ModuleKind.ESNext && moduleKind !== ModuleKind.System && !(moduleKind === ModuleKind.NodeNext && getSourceFileOfNode(node).impliedNodeFormat === ModuleKind.ESNext)) || languageVersion < ScriptTarget.ES2017) {
if ((moduleKind !== ModuleKind.ES2022 && moduleKind !== ModuleKind.ESNext && moduleKind !== ModuleKind.System && !(moduleKind >= ModuleKind.Node16 && getSourceFileOfNode(node).impliedNodeFormat === ModuleKind.ESNext)) || languageVersion < ScriptTarget.ES2017) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this check is wrong and the tests reflect that. Should it be

Suggested change
if ((moduleKind !== ModuleKind.ES2022 && moduleKind !== ModuleKind.ESNext && moduleKind !== ModuleKind.System && !(moduleKind >= ModuleKind.Node16 && getSourceFileOfNode(node).impliedNodeFormat === ModuleKind.ESNext)) || languageVersion < ScriptTarget.ES2017) {
if ((moduleKind !== ModuleKind.ES2022 && moduleKind !== ModuleKind.ESNext && moduleKind !== ModuleKind.System && !(moduleKind < ModuleKind.Node16 && getSourceFileOfNode(node).impliedNodeFormat === ModuleKind.ESNext)) || languageVersion < ScriptTarget.ES2017) {

Copy link
Member

Choose a reason for hiding this comment

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

Normally we still emit warnings of some kind when using experimental features, which would still be in all stable versions of node, at least until nodejs/node#42875 ships in the next version of node 18.

Copy link
Member Author

@DanielRosenwasser DanielRosenwasser Apr 28, 2022

Choose a reason for hiding this comment

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

So maybe until TLA becomes non-experimental we could issue an error like

Top-level 'await' is currently experimental in Node.js. It can only be enabled when the 'module' option is set to 'nodenext'.

Copy link
Member

Choose a reason for hiding this comment

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

On a related note, we should probably remove the experimental warning for json imports in nodenext. Afaik, they've stabilized in node 18. At least the version with the import assertion, anyway. It's contentious among the node maintainers.

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR is already merged - that seems like a pretty strong sign that it will be stable, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

The test wasn't wrong, it was just very confusing because the code was in a CJS file. I've updated the check there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it'll be stable in the next release of node 18, it just hasn't shipped yet, and I don't know if it'll be backported to 16 or not.

Copy link
Member

Choose a reason for hiding this comment

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

At what point do they stop backporting features like that? Maintenance LTS? Is it practical to not create a named moduleResolution target until it solidifies?

Copy link
Member

@weswigham weswigham May 3, 2022

Choose a reason for hiding this comment

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

Changes are backported to LTS versions unless they're somehow breaking from what I understand, while maintenance LTS versions only get security patches. At least I think that's the general policy. (The definition of "breaking" being the somewhat subjective part)

@DanielRosenwasser
Copy link
Member Author

Newest updates give a better error message when you have TLA in a CJS module.

@DanielRosenwasser
Copy link
Member Author

Most recent changes remove the error message for importing a JSON module under Node16 and NodeNext.

@DanielRosenwasser
Copy link
Member Author

A decent amount of stuff seems to already use/support Node 12.

https://cs.github.com/?scopeName=All+repos&scope=&q=%22%5C%22module%5C%22%3A+%5C%22node12%5C%22%22+language%3A%22JSON+with+Comments%22

Should we keep it undocumented and provide an error when using it, but continue supporting it otherwise?

@weswigham
Copy link
Member

weswigham commented Apr 30, 2022

I mean, as I said back in the issue, I'm all for having newer stable versions to match the node versions people wanna target, alongside the existing versions we support, in the same way we build out a new esXXXX target every year (potentially if only to match up that node version to the correct esXXXX target automatically). If we only do even-numbered (and thus LTS) node versions, we should only need to add one every year or so as well.

Sounds like we have fairly good reasons to have both node16 and node18, in addition to 12, since 16 has all those fun es2022 features and 18 has stablized tla and JSON imports, while 12 already has some use.

@DanielRosenwasser
Copy link
Member Author

Spoke with @ahejlsberg and @andrewbranch on this. We could just say Node18 as the initial minimum target which would simplify a bit for us; however, like you've mentioned, Node 18 can get more and it's not yet the active LTS. So let's start with Node 16 and if we see anything that diverges, we can add a Node18 target down the line.

@DanielRosenwasser
Copy link
Member Author

Consensus was on not providing a workaround for nightly-only users. Either you have nightly TS in a lockfile or you've signed yourself up to staying up-to-date.

@weswigham
Copy link
Member

Just remember that any perceived simplicity from initially supporting fewer explicit node versions is very likely a short-lived pipe dream at best - we will have to add more, the same way we add esXXXX targets every year. The kind of big-bang module system update that happened with the esm transition is not how node usually does development, usually you get more minor changes each release, as we can already see between 12, 14, 16, and 18. (And the only reason we didn't see a need to update the commonjs resolver for so long is because all those minor resolver changes were being banked up for the node 12 esm drop - even the cjs ones.) If the issue is the perceived complexity or messaging of many targets, that can is only being kicked, not fundamentally solved.

IMO, a longer term more complete solution would be following in the footsteps of... pretty much every js transforming tool in the js community (eg, babel via preset-env, rollup's target argument), and taking arbitrary engine versions/ranges and mapping them automatically to our internal feature flags (eg, es target, module (resolution), and emit features). That would be more predictable into the future, imo. Messaging is probably simpler: 'specify all desired target runtimes, eg target: node18.1.1,chrome101,firefox102,... and we'll figure out the rest.' where you can even specify future versions we don't explicitly handle yet and we can assume those map to next variants of our various features until we add explicit mappings.

@DanielRosenwasser DanielRosenwasser merged commit 5f9c9a6 into main May 3, 2022
@DanielRosenwasser DanielRosenwasser deleted the toNode16 branch May 3, 2022 23:29
chriskrycho added a commit to true-myth/true-myth that referenced this pull request May 5, 2022
@@ -134,296 +134,33 @@ Output::

[12:00:26 AM] Building project '/src/src-types/tsconfig.json'...

[12:00:33 AM] Project 'src/src-dogs/tsconfig.json' is out of date because output file 'src/src-dogs/dog.js' does not exist
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the tests to modify lib file so there are no errors are we are really testing the scenario.

Copy link
Member

Choose a reason for hiding this comment

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

Created #48974

@@ -16,7 +16,7 @@ export type TheNum = 42;
export type { TheNum } from './const.cjs';

//// [/user/username/projects/myproject/packages/pkg2/tsconfig.json]
{"compilerOptions":{"composite":true,"outDir":"build","module":"node12"}}
{"compilerOptions":{"composite":true,"outDir":"build","module":"node16"}}

//// [/user/username/projects/myproject/packages/pkg2/package.json]
Copy link
Member

Choose a reason for hiding this comment

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

Same for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch from node12 to node16 to avoid top-level await issues
5 participants