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.

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

Make module control impliedNodeFormat and moduleResolution control just module resolution #54788

Closed
wants to merge 9 commits into from
Closed

Make module control impliedNodeFormat and moduleResolution control just module resolution #54788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
660bf60
Make module, not moduleResolution, the trigger for setting impliedNod…
andrewbranch Jun 26, 2023
77a931b
Update baselines
andrewbranch Jun 26, 2023
081a381
Reset target for less baseline change
andrewbranch Jun 26, 2023
d3c08b2
Set lib file content instead of changing target
andrewbranch Jun 26, 2023
34cf76c
Replace lib file instead of changing target
andrewbranch Jun 26, 2023
46572b0
Apply suggestions from code review
andrewbranch Jun 27, 2023
f346be4
Update node10Result baselines
andrewbranch Jun 27, 2023
4e35976
Update src/testRunner/unittests/helpers/node10Result.ts
andrewbranch Jun 27, 2023
4d47e76
Fix node10Result baselines
andrewbranch Jun 27, 2023
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
3 changes: 2 additions & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4883,6 +4883,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
(isLiteralImportTypeNode(location) ? location : undefined)?.argument.literal;
const mode = contextSpecifier && isStringLiteralLike(contextSpecifier) ? getModeForUsageLocation(currentSourceFile, contextSpecifier) : currentSourceFile.impliedNodeFormat;
const moduleResolutionKind = getEmitModuleResolutionKind(compilerOptions);
const moduleKind = getEmitModuleKind(compilerOptions);
const resolvedModule = getResolvedModule(currentSourceFile, moduleReference, mode);
const resolutionDiagnostic = resolvedModule && getResolutionDiagnostic(compilerOptions, resolvedModule, currentSourceFile);
const sourceFile = resolvedModule
Expand Down Expand Up @@ -4914,7 +4915,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (resolvedModule.isExternalLibraryImport && !resolutionExtensionIsTSOrJson(resolvedModule.extension)) {
errorOnImplicitAnyModule(/*isError*/ false, errorNode, currentSourceFile, mode, resolvedModule, moduleReference);
}
if (moduleResolutionKind === ModuleResolutionKind.Node16 || moduleResolutionKind === ModuleResolutionKind.NodeNext) {
if (ModuleKind.Node16 <= moduleKind && moduleKind <= ModuleKind.NodeNext) {
const isSyncImport = (currentSourceFile.impliedNodeFormat === ModuleKind.CommonJS && !findAncestor(location, isImportCall)) || !!findAncestor(location, isImportEqualsDeclaration);
const overrideClauseHost = findAncestor(location, l => isImportTypeNode(l) || isExportDeclaration(l) || isImportDeclaration(l)) as ImportTypeNode | ImportDeclaration | ExportDeclaration | undefined;
const overrideClause = overrideClauseHost && isImportTypeNode(overrideClauseHost) ? overrideClauseHost.assertions?.assertClause : overrideClauseHost?.assertClause;
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1310,7 +1310,7 @@ export interface CreateSourceFileOptions {
/**
* Controls the format the file is detected as - this can be derived from only the path
* and files on disk, but needs to be done with a module resolution cache in scope to be performant.
* This is usually `undefined` for compilations that do not have `moduleResolution` values of `node16` or `nodenext`.
* This is usually `undefined` for compilations that do not have `module` values of `node16` or `nodenext`.
*/
impliedNodeFormat?: ResolutionMode;
/**
Expand Down
8 changes: 4 additions & 4 deletions src/compiler/program.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,7 +883,7 @@ export function isExclusivelyTypeOnlyImportOrExport(decl: ImportDeclaration | Ex
* Calculates the final resolution mode for a given module reference node. This is generally the explicitly provided resolution mode, if
* one exists, or the mode of the containing source file. (Excepting import=require, which is always commonjs, and dynamic import, which is always esm).
* Notably, this function always returns `undefined` if the containing file has an `undefined` `impliedNodeFormat` - this field is only set when
* `moduleResolution` is `node16`+.
* `module` is `node16`+.
* @param file The file the import or import-like reference is contained within
* @param usage The module reference string
* @returns The final resolution mode of the import
Expand Down Expand Up @@ -1315,9 +1315,9 @@ export function getImpliedNodeFormatForFileWorker(
host: ModuleResolutionHost,
options: CompilerOptions,
) {
switch (getEmitModuleResolutionKind(options)) {
case ModuleResolutionKind.Node16:
case ModuleResolutionKind.NodeNext:
switch (getEmitModuleKind(options)) {
case ModuleKind.Node16:
case ModuleKind.NodeNext:
return fileExtensionIsOneOf(fileName, [Extension.Dmts, Extension.Mts, Extension.Mjs]) ? ModuleKind.ESNext :
fileExtensionIsOneOf(fileName, [Extension.Dcts, Extension.Cts, Extension.Cjs]) ? ModuleKind.CommonJS :
fileExtensionIsOneOf(fileName, [Extension.Dts, Extension.Ts, Extension.Tsx, Extension.Js, Extension.Jsx]) ? lookupFromPackageJson() :
Expand Down
6 changes: 4 additions & 2 deletions src/testRunner/unittests/helpers/node10Result.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ export function getFsContentsForNode10Result(): FsContents {
import { foo2 } from "foo2";
import { bar2 } from "bar2";
`,
"/lib/lib.es2022.full.d.ts": libFile.content,
"/home/src/projects/project/tsconfig.json": JSON.stringify({
compilerOptions: {
module: "node16",
moduleResolution: "node16",
traceResolution: true,
incremental: true,
Expand All @@ -80,6 +82,6 @@ export function getFsContentsForNode10Result(): FsContents {
},
files: ["index.mts"]
}),
[libFile.path]: libFile.content,
[libFile.path.replace("lib.d.ts", "lib.es2022.full.d.ts")]: libFile.content,
};
}
}
3 changes: 2 additions & 1 deletion src/testRunner/unittests/tscWatch/forceConsistentCasingInFileNames.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,13 @@ a;b;
}),
"/Users/name/projects/web/tsconfig.json": JSON.stringify({
compilerOptions: {
module: "nodenext",
moduleResolution: "nodenext",
forceConsistentCasingInFileNames: true,
traceResolution: true,
}
}),
[libFile.path]: libFile.content,
[libFile.path.replace("lib.d.ts", "lib.esnext.full.d.ts")]: libFile.content,
}, { currentDirectory: "/Users/name/projects/web" }),
});

Expand Down
17 changes: 12 additions & 5 deletions src/testRunner/unittests/tscWatch/moduleResolution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
path: `/user/username/projects/myproject/tsconfig.json`,
content: JSON.stringify({
compilerOptions: {
module: "nodenext",
moduleResolution: "nodenext",
outDir: "./dist",
declaration: true,
Expand Down Expand Up @@ -123,7 +124,10 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
export function thing(): void {}
`
},
libFile
{
...libFile,
path: libFile.path.replace("lib.d.ts", "lib.esnext.full.d.ts")
}
], { currentDirectory: "/user/username/projects/myproject" }),
commandLineArgs: ["-w", "--traceResolution"],
edits: [{
Expand Down Expand Up @@ -282,7 +286,7 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
{
path: `/user/username/projects/myproject/tsconfig.json`,
content: JSON.stringify({
compilerOptions: { moduleResolution: "node16" },
compilerOptions: { target: "es5", module: "node16", moduleResolution: "node16" },
})
},
{
Expand Down Expand Up @@ -352,7 +356,7 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
{
path: `/user/username/projects/myproject/tsconfig.json`,
content: JSON.stringify({
compilerOptions: { moduleResolution: "node16" },
compilerOptions: { module: "node16", moduleResolution: "node16" },
})
},
{
Expand Down Expand Up @@ -422,7 +426,10 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
path: `/user/username/projects/myproject/node_modules/@types/pkg2/index.d.ts`,
content: `export const x = 10;`
},
libFile
{
...libFile,
path: libFile.path.replace("lib.d.ts", "lib.es2022.full.d.ts"),
}
], { currentDirectory: "/user/username/projects/myproject" }),
commandLineArgs: ["-w", "--traceResolution"],
edits: [
Expand Down Expand Up @@ -538,4 +545,4 @@ describe("unittests:: tsc-watch:: moduleResolution", () => {
},
]
});
});
});
4 changes: 2 additions & 2 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9191,7 +9191,7 @@ declare namespace ts {
/**
* Controls the format the file is detected as - this can be derived from only the path
* and files on disk, but needs to be done with a module resolution cache in scope to be performant.
* This is usually `undefined` for compilations that do not have `moduleResolution` values of `node16` or `nodenext`.
* This is usually `undefined` for compilations that do not have `module` values of `node16` or `nodenext`.
*/
impliedNodeFormat?: ResolutionMode;
/**
Expand Down Expand Up @@ -9504,7 +9504,7 @@ declare namespace ts {
* Calculates the final resolution mode for a given module reference node. This is generally the explicitly provided resolution mode, if
* one exists, or the mode of the containing source file. (Excepting import=require, which is always commonjs, and dynamic import, which is always esm).
* Notably, this function always returns `undefined` if the containing file has an `undefined` `impliedNodeFormat` - this field is only set when
* `moduleResolution` is `node16`+.
* `module` is `node16`+.
* @param file The file the import or import-like reference is contained within
* @param usage The module reference string
* @returns The final resolution mode of the import
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5138,7 +5138,7 @@ declare namespace ts {
/**
* Controls the format the file is detected as - this can be derived from only the path
* and files on disk, but needs to be done with a module resolution cache in scope to be performant.
* This is usually `undefined` for compilations that do not have `moduleResolution` values of `node16` or `nodenext`.
* This is usually `undefined` for compilations that do not have `module` values of `node16` or `nodenext`.
*/
impliedNodeFormat?: ResolutionMode;
/**
Expand Down Expand Up @@ -5451,7 +5451,7 @@ declare namespace ts {
* Calculates the final resolution mode for a given module reference node. This is generally the explicitly provided resolution mode, if
* one exists, or the mode of the containing source file. (Excepting import=require, which is always commonjs, and dynamic import, which is always esm).
* Notably, this function always returns `undefined` if the containing file has an `undefined` `impliedNodeFormat` - this field is only set when
* `moduleResolution` is `node16`+.
* `module` is `node16`+.
* @param file The file the import or import-like reference is contained within
* @param usage The module reference string
* @returns The final resolution mode of the import
Expand Down
2 changes: 2 additions & 0 deletions ...lines/reference/conditionalExportsResolutionFallback(moduleresolution=bundler).errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.
error TS5109: Option 'moduleResolution' must be set to 'NodeNext' (or left unspecified) when option 'module' is set to 'NodeNext'.


!!! error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.
Copy link
Member

Choose a reason for hiding this comment

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

Wait... so module now controls weather we interpret files as cjs/esm, but you still can't combine that with bundler? I thought the point of this was basically to make it so bundler could assimilate the mode-swapping interpretation? So you could mimic bundlers that more closely mimiced node?

Isn't allowing module: nodenext + moduleResultion: bundler for modern-node-like bundler environments kinda the goal here?

Copy link
Member

Choose a reason for hiding this comment

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

(Since this format data directly controls, eg, how we interpret default imports, the distinction may be material)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's the linked issue #54102 - shouldn't we just fix that, while we're here? It should be removing this error, 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.

Yeah, that is the ultimate goal, but I don’t think --module nodenext is the right thing to combine with --moduleResolution bundler. --module nodenext implies that CJS-mode files actually get emitted as CJS, which doesn’t make sense in the context of most bundlers. In bundlers, the choice is not between CJS and ESM; the default is ESM with Babel-like interop and .mjs files just disable the Babel-like interop in favor of Node’s always-synthesize-a-default interop. It can be hard to tell the difference between “ESM with Babel-like interop” and “it’s actually just CJS with esModuleInterop under the hood,” but we can tell there’s a difference because

  • top-level wait is always allowed (by those that support it at all)
  • the module format does not affect the "imports"/"exports" conditions used in module resolution—import statements always have "import"; they’re not secretly transformed to require first in CJS-mode files
  • some bundlers refuse to process require calls outside of node_modules at all

Additionally, the transform for import foo = require(...) to a createRequire call under --module nodenext is highly Node-specific and would fail under I think every bundler (maybe Webpack has compat for that; they do the most complete job at imitating Node).

So, my thinking for addressing #54102 is that either there needs to be a different module mode for these bundlers that have Node-like interop behavior (in which case this PR is necessary but not sufficient), or the thing that makes us set impliedNodeFormat in the first place needs to be decoupled from module (in which case this PR is not necessary, but doesn’t hurt anything as an interim step toward the eventual goal). Either way, we’ll need to move it off of moduleResolution in order to resolve #54102, and module will imply an impliedNodeFormat-setting behavior if it doesn’t directly control it.

!!! error TS5109: Option 'moduleResolution' must be set to 'NodeNext' (or left unspecified) when option 'module' is set to 'NodeNext'.
==== /node_modules/dep/package.json (0 errors) ====
// This documents bug https://github.com/microsoft/TypeScript/issues/50762.

Expand Down
20 changes: 18 additions & 2 deletions ...lines/reference/conditionalExportsResolutionFallback(moduleresolution=bundler).trace.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
[
"File '/node_modules/dep/dist/package.json' does not exist.",
"Found 'package.json' at '/node_modules/dep/package.json'.",
"======== Resolving module 'dep' from '/index.mts'. ========",
"Explicitly specified module resolution kind: 'Bundler'.",
"Resolving in CJS mode with conditions 'import', 'types'.",
"File '/package.json' does not exist.",
"Loading module 'dep' from 'node_modules' folder, target file types: TypeScript, JavaScript, Declaration, JSON.",
"Searching all ancestor node_modules directories for preferred extensions: TypeScript, Declaration.",
"Found 'package.json' at '/node_modules/dep/package.json'.",
"File '/node_modules/dep/package.json' exists according to earlier cached lookups.",
"Entering conditional exports.",
"Matched 'exports' condition 'import'.",
"Using 'exports' subpath '.' with target './dist/index.mjs'.",
Expand All @@ -21,6 +23,8 @@
"Exiting conditional exports.",
"Resolving real path for '/node_modules/dep/dist/index.d.ts', result '/node_modules/dep/dist/index.d.ts'.",
"======== Module name 'dep' was successfully resolved to '/node_modules/dep/dist/index.d.ts' with Package ID 'dep/dist/[email protected]'. ========",
"File '/.ts/package.json' does not exist.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-es5' from '/.src/__lib_node_modules_lookup_lib.es5.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-es5' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -33,6 +37,8 @@
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-es5' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-decorators' from '/.src/__lib_node_modules_lookup_lib.decorators.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-decorators' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -45,6 +51,8 @@
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-decorators' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-decorators/legacy' from '/.src/__lib_node_modules_lookup_lib.decorators.legacy.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-decorators/legacy' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -57,6 +65,8 @@
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-decorators/legacy' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-dom' from '/.src/__lib_node_modules_lookup_lib.dom.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-dom' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -69,6 +79,8 @@
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-dom' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-webworker/importscripts' from '/.src/__lib_node_modules_lookup_lib.webworker.importscripts.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-webworker/importscripts' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -81,6 +93,8 @@
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-webworker/importscripts' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups.",
"======== Resolving module '@typescript/lib-scripthost' from '/.src/__lib_node_modules_lookup_lib.scripthost.d.ts__.ts'. ========",
"Explicitly specified module resolution kind: 'Node10'.",
"Loading module '@typescript/lib-scripthost' from 'node_modules' folder, target file types: TypeScript, Declaration.",
Expand All @@ -92,5 +106,7 @@
"Loading module '@typescript/lib-scripthost' from 'node_modules' folder, target file types: JavaScript.",
"Searching all ancestor node_modules directories for fallback extensions: JavaScript.",
"Directory '/.src/node_modules' does not exist, skipping all lookups in it.",
"======== Module name '@typescript/lib-scripthost' was not resolved. ========"
"======== Module name '@typescript/lib-scripthost' was not resolved. ========",
"File '/.ts/package.json' does not exist according to earlier cached lookups.",
"File '/package.json' does not exist according to earlier cached lookups."
]
31 changes: 0 additions & 31 deletions ...elines/reference/conditionalExportsResolutionFallback(moduleresolution=node16).errors.txt
View file
Open in desktop

This file was deleted.

31 changes: 0 additions & 31 deletions ...ines/reference/conditionalExportsResolutionFallback(moduleresolution=nodenext).errors.txt
View file
Open in desktop

This file was deleted.

Loading