- Notifications
You must be signed in to change notification settings - Fork12.9k
Add--module node18#60722
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add--module node18#60722
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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. |
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document themon our wiki's API Breaking Changes page. Also, please make sure@DanielRosenwasser and@RyanCavanaugh are aware of the changes, just as a heads up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Compare this file...
There was a problem hiding this comment.
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
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.txtindex 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? |
andrewbranch commentedDec 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Yeah, that is preexisting, but a good point. However, good workarounds exist ( |
Well, you can't write Just to be clear, is it preexisting as in "always", or preexisting in 5.7 specifically due to our added checks for attributes? |
andrewbranch commentedDec 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
That’s not true; This looks like a new problemfor 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. |
Wow. I had no idea we did this: importblah= require("react");console.log(blah); import{createRequireas_createRequire}from"module";const__require=_createRequire(import.meta.url);constblah=__require("react");console.log(blah); |
| "use strict"; | ||
| Object.defineProperty(exports, "__esModule", { value: true }); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lol, the value is123 which is in the Node16–NodeNext range.
| @@ -1,4 +1,4 @@ | |||
| // @moduleResolution: classic, node, node16, nodenext, bundler | |||
| // @moduleResolution: classic, node, node16,nodenext, bundler | |||
There was a problem hiding this comment.
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)
There was a problem hiding this 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.
f69580f intomicrosoft:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Identical to
nodenext, except implies--target es2022likenode16does.I validated the new baselines with
and
Closes#60705