Skip to content

Start Node ESM stable version at Node16 #48879

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.

Sign up for GitHub

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

Merged
DanielRosenwasser merged 12 commits into from
May 3, 2022
Merged

Start Node ESM stable version at Node16 #48879

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c0f2882
Remove Node12, add Node16.
DanielRosenwasser Apr 28, 2022
52d1ea5
Accepted baselines.
DanielRosenwasser Apr 28, 2022
1247217
Refactor checking for top-level await, give a better error message in…
DanielRosenwasser Apr 29, 2022
c390b79
Accepted baselines.
DanielRosenwasser Apr 29, 2022
8b1e656
Stop erroring on JSON module imports in node ESM since they're no lon…
DanielRosenwasser Apr 29, 2022
ce98f0a
Accepted baselines.
DanielRosenwasser Apr 29, 2022
fac5ded
More refactoring, do the same checks for for-await loops.
DanielRosenwasser Apr 29, 2022
5305796
Accepted baselines.
DanielRosenwasser Apr 29, 2022
1ca971d
Adjust phrasing to permit for-await on CJS error.
DanielRosenwasser Apr 29, 2022
4c46f26
Accepted baselines.
DanielRosenwasser Apr 29, 2022
830871c
Accepted baselines.
DanielRosenwasser Apr 29, 2022
9d7fae8
Fix lints.
DanielRosenwasser Apr 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
More refactoring, do the same checks for for-await loops.
  • Loading branch information
@DanielRosenwasser
DanielRosenwasser authored Apr 29, 2022
commit fac5dedf47b46d166650e4ee9efdb3f86cd8fab5
29 changes: 25 additions & 4 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33101,7 +33101,7 @@ namespace ts {
switch (moduleKind) {
case ModuleKind.Node16:
case ModuleKind.NodeNext:
if (getSourceFileOfNode(node).impliedNodeFormat === ModuleKind.CommonJS) {
if (sourceFile.impliedNodeFormat === ModuleKind.CommonJS) {
Copy link
Copy Markdown
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.

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
Copy Markdown
Member

@weswigham weswigham Apr 28, 2022

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
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Apr 28, 2022 •
edited
Loading

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
Copy Markdown
Member

@weswigham weswigham Apr 28, 2022

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
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Apr 29, 2022

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
Copy Markdown
Member Author

@DanielRosenwasser DanielRosenwasser Apr 29, 2022

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
Copy Markdown
Member

@weswigham weswigham Apr 29, 2022

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
Copy Markdown
Member

@andrewbranch andrewbranch May 2, 2022

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
Copy Markdown
Member

@weswigham weswigham May 3, 2022 •
edited
Loading

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)

span ??= getSpanOfTokenAtPosition(sourceFile, node.pos)
diagnostics.add(
createFileDiagnostic(sourceFile, span.start, span.length, Diagnostics.The_current_file_is_a_CommonJS_module_and_cannot_use_await_expressions_at_the_top_level)
Expand Down Expand Up @@ -44191,9 +44191,30 @@ namespace ts {
diagnostics.add(createDiagnosticForNode(forInOrOfStatement.awaitModifier,
Diagnostics.for_await_loops_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));
}
if ((moduleKind !== ModuleKind.ES2022 && moduleKind !== ModuleKind.ESNext && moduleKind !== ModuleKind.System && !(moduleKind >= ModuleKind.Node16 && getSourceFileOfNode(forInOrOfStatement).impliedNodeFormat === ModuleKind.ESNext)) || languageVersion < ScriptTarget.ES2017) {
diagnostics.add(createDiagnosticForNode(forInOrOfStatement.awaitModifier,
Diagnostics.Top_level_for_await_loops_are_only_allowed_when_the_module_option_is_set_to_es2022_esnext_system_node16_or_nodenext_and_the_target_option_is_set_to_es2017_or_higher));
switch (moduleKind) {
case ModuleKind.Node16:
case ModuleKind.NodeNext:
if (sourceFile.impliedNodeFormat === ModuleKind.CommonJS) {
diagnostics.add(
createDiagnosticForNode(forInOrOfStatement.awaitModifier, Diagnostics.The_current_file_is_a_CommonJS_module_and_cannot_use_await_expressions_at_the_top_level)
);
break;
}
// fallthrough
case ModuleKind.ES2022:
case ModuleKind.ESNext:
case ModuleKind.System:
if (languageVersion >= ScriptTarget.ES2017) {
break;
}
// fallthrough
default:
diagnostics.add(
createDiagnosticForNode(forInOrOfStatement.awaitModifier,
Diagnostics.Top_level_for_await_loops_are_only_allowed_when_the_module_option_is_set_to_es2022_esnext_system_node16_or_nodenext_and_the_target_option_is_set_to_es2017_or_higher
)
);
break;
}
}
}
Expand Down