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

Add --module node18 #60722

Merged
merged 8 commits into from
Dec 13, 2024
Merged

Add --module node18 #60722

merged 8 commits into from
Dec 13, 2024

Conversation

andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Dec 9, 2024

Identical to nodenext, except implies --target es2022 like node16 does.

I validated the new baselines with

for f in tests/baselines/reference/*module=node18*; do
  [[ -e ${f/module=node18/module=node16} ]] && echo "$f" && diff "$f" "${f/module=node18/module=node16}";
done

and

for f in tests/baselines/reference/*module=node18*; do
  [[ -e ${f/module=node18/module=nodenext} ]] && echo "$f" && diff "$f" "${f/module=node18/module=nodenext}";
done

Closes #60705

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Dec 9, 2024
@typescript-bot
Copy link
Collaborator

Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary.

@typescript-bot
Copy link
Collaborator

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Compare this file...

Copy link
Member Author

Choose a reason for hiding this comment

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

...with this one to see the point of this PR

@andrewbranch andrewbranch marked this pull request as ready for review December 9, 2024 23:42
@jakebailey
Copy link
Member

The example you gave has a diff like:

diff --git a/tests/baselines/reference/nodeModulesJson(module=node16).errors.txt b/tests/baselines/reference/nodeModulesJson(module=node18).errors.txt
index b3f36d73fa..033a1c9f7c 100644
--- a/tests/baselines/reference/nodeModulesJson(module=node16).errors.txt
+++ b/tests/baselines/reference/nodeModulesJson(module=node18).errors.txt
@@ -1,10 +1,9 @@
-/loosey.cts(1,36): error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
+/loosey.cts(1,36): error TS2856: Import attributes are not allowed on statements that compile to CommonJS 'require' calls.
 /loosey.cts(6,9): error TS2339: Property 'default' does not exist on type '{ version: number; }'.
-/main.mts(5,36): error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
-/main.mts(6,52): error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
-/main.mts(8,10): error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'Node16'.
-/main.mts(8,41): error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
-/main.mts(9,42): error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
+/main.mts(2,22): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
+/main.mts(3,19): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
+/main.mts(7,21): error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
+/main.mts(8,10): error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'Node18'.
 /main.mts(10,9): error TS2339: Property 'version' does not exist on type '{ default: { version: number; }; }'.
 
 
@@ -42,26 +41,24 @@
       "version": 1
     }
     
-==== /main.mts (6 errors) ====
+==== /main.mts (5 errors) ====
     import { oops } from "not.json"; // Ok
     import moreOops from "actually-json"; // Error in nodenext
+                         ~~~~~~~~~~~~~~~
+!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
     import typed from "actually-json/typed"; // Error in nodenext
+                      ~~~~~~~~~~~~~~~~~~~~~
+!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
     
     import config from "./config.json" with { type: "json" }; // Ok
-                                       ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
     import { default as config1 } from "./config.json" with { type: "json" }; // Ok
-                                                       ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
     import config2 from "./config.json"; // Error in nodenext, no attribute
+                        ~~~~~~~~~~~~~~~
+!!! error TS1543: Importing a JSON file into an ECMAScript module requires a 'type: "json"' import attribute when 'module' is set to 'Node18'.
     import { version } from "./config.json" with { type: "json" }; // Error, named import
              ~~~~~~~
-!!! error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'Node16'.
-                                            ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
+!!! error TS1544: Named imports from a JSON file into an ECMAScript module are not allowed when 'module' is set to 'Node18'.
     import * as config3 from "./config.json" with { type: "json" };
-                                             ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
     config3.version; // Error
             ~~~~~~~
 !!! error TS2339: Property 'version' does not exist on type '{ default: { version: number; }; }'.
@@ -70,7 +67,7 @@
 ==== /loosey.cts (2 errors) ====
     import config from "./config.json" with { type: "json" }; // Error
                                        ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
+!!! error TS2856: Import attributes are not allowed on statements that compile to CommonJS 'require' calls.
     import config2 from "./config.json"; // Ok
     import { version } from "./config.json"; // Ok
     import * as config3 from "./config.json";

Maybe this is preexisting, but with:

@@ -70,7 +67,7 @@
 ==== /loosey.cts (2 errors) ====
     import config from "./config.json" with { type: "json" }; // Error
                                        ~~~~~~~~~~~~~~~~~~~~~
-!!! error TS2823: Import attributes are only supported when the '--module' option is set to 'esnext', 'node18', 'nodenext', or 'preserve'.
+!!! error TS2856: Import attributes are not allowed on statements that compile to CommonJS 'require' calls.
     import config2 from "./config.json"; // Ok

If someone wants to dual emit CJS/ESM but needs to write an attribute for JSON, are they out of luck? require allows JSON so I would have sort of expected us to say "okay" to this. But maybe it was already this restrictive?

@andrewbranch
Copy link
Member Author

andrewbranch commented Dec 11, 2024

Yeah, that is preexisting, but a good point. However, good workarounds exist (import = and fs.readFile), so I’m not sure we need to stress about it.

@jakebailey
Copy link
Member

Well, you can't write import = if you want to be able to emit ESM, but you can of course always just read the file from the FS (ignoring potential emit path differences).

Just to be clear, is it preexisting as in "always", or preexisting in 5.7 specifically due to our added checks for attributes?

@andrewbranch
Copy link
Member Author

andrewbranch commented Dec 11, 2024

That’s not true; import = compiles to createRequire(import.meta.url)(...) for Node.js module targets.

This looks like a new problem for us because of the new checks, but Node.js has enforced import assertions/attributes on JSON imports even while we have not, so we aren’t closing any doors with this. Anyone trying to do this previously has already had to work around it for runtime reasons, without help from TS.

@jakebailey
Copy link
Member

Wow. I had no idea we did this:

import blah = require("react");

console.log(blah);
import { createRequire as _createRequire } from "module";
const __require = _createRequire(import.meta.url);
const blah = __require("react");
console.log(blah);

Comment on lines +1 to +2
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
Copy link
Member

Choose a reason for hiding this comment

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

This seems like an interesting case; if the module kind is out of range, how are we ending up with CJS?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lol, the value is 123 which is in the Node16–NodeNext range.

@@ -1,4 +1,4 @@
// @moduleResolution: classic, node, node16, nodenext, bundler
// @moduleResolution: classic, node, node16, nodenext, bundler
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a typo (not that it matters)

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Otherwise I think this is good.

@andrewbranch andrewbranch merged commit f69580f into microsoft:main Dec 13, 2024
32 checks passed
@andrewbranch andrewbranch deleted the module-node18 branch December 13, 2024 18:16
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.

Add --module node18
3 participants