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

--moduleResolution bundler: Require ESM for module and remove node from hard-coded conditions #52940

Merged
merged 3 commits into from
Feb 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 6 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5019,7 +5019,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
* @see https://github.com/microsoft/TypeScript/issues/42151
*/
if (emitModuleKindIsNonNodeESM(moduleKind) || mode === ModuleKind.ESNext) {
return importSourceWithoutExtension + (tsExtension === Extension.Mts ? ".mjs" : tsExtension === Extension.Cts ? ".cjs" : ".js");
const preferTs = isDeclarationFileName(moduleReference) && shouldAllowImportingTsExtension(compilerOptions);
const ext =
tsExtension === Extension.Mts || tsExtension === Extension.Dmts ? preferTs ? ".mts" : ".mjs" :
tsExtension === Extension.Cts || tsExtension === Extension.Dmts ? preferTs ? ".cts" : ".cjs" :
preferTs ? ".ts" : ".js";
return importSourceWithoutExtension + ext;
}
Comment on lines 5021 to 5028
Copy link
Member Author

Choose a reason for hiding this comment

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

It has never made a ton of sense to predicate adding an extension to the suggested import source based on module and not moduleResolution, but rather than rethink that whole issue, I just made a narrow fix for when .ts is allowed and intended but the message suggests .js.

return importSourceWithoutExtension;
}
Expand Down Expand Up @@ -43963,9 +43968,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// Import equals declaration is deprecated in es6 or above
grammarErrorOnNode(node, Diagnostics.Import_assignment_cannot_be_used_when_targeting_ECMAScript_modules_Consider_using_import_Asterisk_as_ns_from_mod_import_a_from_mod_import_d_from_mod_or_another_module_format_instead);
}
else if (!(node.flags & NodeFlags.Ambient) && getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.Bundler) {
grammarErrorOnNode(node, Diagnostics.Import_assignment_is_not_allowed_when_moduleResolution_is_set_to_bundler_Consider_using_import_Asterisk_as_ns_from_mod_import_a_from_mod_import_d_from_mod_or_another_module_format_instead);
}
}
Comment on lines -43966 to 43971
Copy link
Member Author

Choose a reason for hiding this comment

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

We can drop this error because it’s redundant with the one that already existed for using import= in --module esnext. That makes me feel even more like this is a coherent move.

}
}
Expand Down Expand Up @@ -44208,9 +44210,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// system modules does not support export assignment
grammarErrorOnNode(node, Diagnostics.Export_assignment_is_not_supported_when_module_flag_is_system);
}
else if (getEmitModuleResolutionKind(compilerOptions) === ModuleResolutionKind.Bundler && !(node.flags & NodeFlags.Ambient)) {
grammarErrorOnNode(node, Diagnostics.Export_assignment_cannot_be_used_when_moduleResolution_is_set_to_bundler_Consider_using_export_default_or_another_module_format_instead);
}
}
}

Expand Down
10 changes: 1 addition & 9 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -4265,7 +4265,7 @@
"category": "Error",
"code": 5094
},
"Option 'preserveValueImports' can only be used when 'module' is set to 'es2015' or later.": {
"Option '{0}' can only be used when 'module' is set to 'es2015' or later.": {
"category": "Error",
"code": 5095
},
Expand All @@ -4281,14 +4281,6 @@
"category": "Error",
"code": 5098
},
"Import assignment is not allowed when 'moduleResolution' is set to 'bundler'. Consider using 'import * as ns from \"mod\"', 'import {a} from \"mod\"', 'import d from \"mod\"', or another module format instead.": {
"category": "Error",
"code": 5099
},
"Export assignment cannot be used when 'moduleResolution' is set to 'bundler'. Consider using 'export default' or another module format instead.": {
"category": "Error",
"code": 5100
},
"Flag '{0}' is deprecated and will stop functioning in TypeScript {1}. Specify compilerOption '\"ignoreDeprecations\": \"{2}\"' to silence this error.": {
"category": "Error",
"code": 5101
Expand Down
10 changes: 7 additions & 3 deletions src/compiler/moduleNameResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import {
ModuleResolutionHost,
ModuleResolutionKind,
moduleResolutionOptionDeclarations,
moduleResolutionSupportsPackageJsonExportsAndImports,
noop,
noopPush,
normalizePath,
Expand Down Expand Up @@ -705,11 +706,14 @@ export function getConditions(options: CompilerOptions, esmMode?: boolean) {
// conditions are only used by the node16/nodenext/bundler resolvers - there's no priority order in the list,
// it's essentially a set (priority is determined by object insertion order in the object we look at).
const conditions = esmMode || getEmitModuleResolutionKind(options) === ModuleResolutionKind.Bundler
? ["node", "import"]
: ["node", "require"];
? ["import"]
: ["require"];
if (!options.noDtsResolution) {
conditions.push("types");
}
if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.Bundler) {
conditions.push("node");
}
return concatenate(conditions, options.customConditions);
}

Expand Down Expand Up @@ -1701,7 +1705,7 @@ function nodeModuleNameResolverWorker(features: NodeResolutionFeatures, moduleNa
candidateIsFromPackageJsonField: false,
};

if (traceEnabled && getEmitModuleResolutionKind(compilerOptions) >= ModuleResolutionKind.Node16 && getEmitModuleResolutionKind(compilerOptions) <= ModuleResolutionKind.NodeNext) {
if (traceEnabled && moduleResolutionSupportsPackageJsonExportsAndImports(getEmitModuleResolutionKind(compilerOptions))) {
trace(host, Diagnostics.Resolving_in_0_mode_with_conditions_1, features & NodeResolutionFeatures.EsmMode ? "ESM" : "CJS", conditions.map(c => `'${c}'`).join(", "));
}

Expand Down
15 changes: 10 additions & 5 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ import {
DirectoryStructureHost,
emitFiles,
EmitHost,
emitModuleKindIsNonNodeESM,
EmitOnly,
EmitResult,
emptyArray,
Expand Down Expand Up @@ -4241,11 +4242,11 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg
}

if (options.preserveValueImports && getEmitModuleKind(options) < ModuleKind.ES2015) {
createDiagnosticForOptionName(Diagnostics.Option_preserveValueImports_can_only_be_used_when_module_is_set_to_es2015_or_later, "preserveValueImports");
createDiagnosticForOptionName(Diagnostics.Option_0_can_only_be_used_when_module_is_set_to_es2015_or_later, "preserveValueImports");
}

const moduleKind = getEmitModuleKind(options);
if (options.verbatimModuleSyntax) {
const moduleKind = getEmitModuleKind(options);
if (moduleKind === ModuleKind.AMD || moduleKind === ModuleKind.UMD || moduleKind === ModuleKind.System) {
createDiagnosticForOptionName(Diagnostics.Option_verbatimModuleSyntax_cannot_be_used_when_module_is_set_to_UMD_AMD_or_System, "verbatimModuleSyntax");
}
Expand All @@ -4266,13 +4267,17 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg

const moduleResolution = getEmitModuleResolutionKind(options);
if (options.resolvePackageJsonExports && !moduleResolutionSupportsPackageJsonExportsAndImports(moduleResolution)) {
createOptionValueDiagnostic("resolvePackageJsonExports", Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "resolvePackageJsonExports");
createDiagnosticForOptionName(Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "resolvePackageJsonExports");
}
if (options.resolvePackageJsonImports && !moduleResolutionSupportsPackageJsonExportsAndImports(moduleResolution)) {
createOptionValueDiagnostic("resolvePackageJsonImports", Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "resolvePackageJsonImports");
createDiagnosticForOptionName(Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "resolvePackageJsonImports");
}
if (options.customConditions && !moduleResolutionSupportsPackageJsonExportsAndImports(moduleResolution)) {
createOptionValueDiagnostic("customConditions", Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "customConditions");
createDiagnosticForOptionName(Diagnostics.Option_0_can_only_be_used_when_moduleResolution_is_set_to_node16_nodenext_or_bundler, "customConditions");
}

if (moduleResolution === ModuleResolutionKind.Bundler && !emitModuleKindIsNonNodeESM(moduleKind)) {
createOptionValueDiagnostic("moduleResolution", Diagnostics.Option_0_can_only_be_used_when_module_is_set_to_es2015_or_later, "bundler");
}

// If the emit is enabled make sure that every output file is unique and not overwriting any of the input files
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.
/c.ts(1,16): error TS2307: Cannot find module './thisfiledoesnotexist.ts' or its corresponding type declarations.


!!! error TS5095: Option 'bundler' can only be used when 'module' is set to 'es2015' or later.
==== /ts.ts (0 errors) ====
export {};

Expand Down
44 changes: 44 additions & 0 deletions tests/baselines/reference/bundlerConditionsExcludesNode.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
error TS6504: File '/node_modules/conditions/index.node.js' is a JavaScript file. Did you mean to enable the 'allowJs' option?
The file is in the program because:
Root file specified for compilation
error TS6504: File '/node_modules/conditions/index.web.js' is a JavaScript file. Did you mean to enable the 'allowJs' option?
The file is in the program because:
Root file specified for compilation


!!! error TS6504: File '/node_modules/conditions/index.node.js' is a JavaScript file. Did you mean to enable the 'allowJs' option?
!!! error TS6504: The file is in the program because:
!!! error TS6504: Root file specified for compilation
!!! error TS6504: File '/node_modules/conditions/index.web.js' is a JavaScript file. Did you mean to enable the 'allowJs' option?
!!! error TS6504: The file is in the program because:
!!! error TS6504: Root file specified for compilation
==== /node_modules/conditions/package.json (0 errors) ====
{
"name": "conditions",
"version": "1.0.0",
"type": "module",
"main": "index.cjs",
"types": "index.d.cts",
"exports": {
".": {
"node": "./index.node.js",
"default": "./index.web.js"
}
}
}

==== /node_modules/conditions/index.node.js (0 errors) ====
export const node = 0;

==== /node_modules/conditions/index.node.d.ts (0 errors) ====
export const node: number;

==== /node_modules/conditions/index.web.js (0 errors) ====
export const web = 0;

==== /node_modules/conditions/index.web.d.ts (0 errors) ====
export const web: number;

==== /main.ts (0 errors) ====
import { web } from "conditions";

35 changes: 35 additions & 0 deletions tests/baselines/reference/bundlerConditionsExcludesNode.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//// [tests/cases/conformance/moduleResolution/bundler/bundlerConditionsExcludesNode.ts] ////

//// [package.json]
{
"name": "conditions",
"version": "1.0.0",
"type": "module",
"main": "index.cjs",
"types": "index.d.cts",
"exports": {
".": {
"node": "./index.node.js",
"default": "./index.web.js"
}
}
}

//// [index.node.js]
export const node = 0;

//// [index.node.d.ts]
export const node: number;

//// [index.web.js]
export const web = 0;

//// [index.web.d.ts]
export const web: number;

//// [main.ts]
import { web } from "conditions";


//// [main.js]
export {};
12 changes: 12 additions & 0 deletions tests/baselines/reference/bundlerConditionsExcludesNode.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== /node_modules/conditions/index.node.d.ts ===
export const node: number;
>node : Symbol(node, Decl(index.node.d.ts, 0, 12))

=== /node_modules/conditions/index.web.d.ts ===
export const web: number;
>web : Symbol(web, Decl(index.web.d.ts, 0, 12))

=== /main.ts ===
import { web } from "conditions";
>web : Symbol(web, Decl(main.ts, 0, 8))

20 changes: 20 additions & 0 deletions tests/baselines/reference/bundlerConditionsExcludesNode.trace.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
[
"======== Resolving module 'conditions' from '/main.ts'. ========",
"Explicitly specified module resolution kind: 'Bundler'.",
"Resolving in CJS mode with conditions 'import', 'types'.",
"File '/package.json' does not exist.",
"Loading module 'conditions' from 'node_modules' folder, target file types: TypeScript, JavaScript, Declaration, JSON.",
"Found 'package.json' at '/node_modules/conditions/package.json'.",
"Entering conditional exports.",
"Saw non-matching condition 'node'.",
"Matched 'exports' condition 'default'.",
"Using 'exports' subpath '.' with target './index.web.js'.",
"File name '/node_modules/conditions/index.web.js' has a '.js' extension - stripping it.",
"File '/node_modules/conditions/index.web.ts' does not exist.",
"File '/node_modules/conditions/index.web.tsx' does not exist.",
"File '/node_modules/conditions/index.web.d.ts' exists - use it as a name resolution result.",
"Resolved under condition 'default'.",
"Exiting conditional exports.",
"Resolving real path for '/node_modules/conditions/index.web.d.ts', result '/node_modules/conditions/index.web.d.ts'.",
"======== Module name 'conditions' was successfully resolved to '/node_modules/conditions/index.web.d.ts' with Package ID 'conditions/[email protected]'. ========"
]
12 changes: 12 additions & 0 deletions tests/baselines/reference/bundlerConditionsExcludesNode.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== /node_modules/conditions/index.node.d.ts ===
export const node: number;
>node : number

=== /node_modules/conditions/index.web.d.ts ===
export const web: number;
>web : number

=== /main.ts ===
import { web } from "conditions";
>web : number

11 changes: 3 additions & 8 deletions tests/baselines/reference/bundlerImportESM.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,8 @@ import { esm } from "./esm.mjs";


//// [esm.mjs]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.esm = void 0;
exports.esm = 0;
export var esm = 0;
//// [not-actually-cjs.cjs]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
//// [still-not-cjs.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ error TS6054: File '/project/e.txt' has an unsupported extension. The only suppo
Root file specified for compilation
/project/main.ts(3,16): error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.
/project/main.ts(7,16): error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.
/project/main.ts(8,16): error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './b' instead?
/project/main.ts(8,16): error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './b.js' instead?
/project/main.ts(11,16): error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.
/project/main.ts(12,16): error TS5097: An import path can only end with a '.tsx' extension when 'allowImportingTsExtensions' is enabled.
/project/main.ts(12,16): error TS6142: Module './c.tsx' was resolved to '/project/c.tsx', but '--jsx' is not set.
/project/main.ts(16,16): error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.
/project/types.d.ts(2,16): error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a' instead?
/project/types.d.ts(2,16): error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a.js' instead?


!!! error TS5056: Cannot write file 'out/b.js' because it would be overwritten by multiple input files.
Expand Down Expand Up @@ -68,7 +68,7 @@ error TS6054: File '/project/e.txt' has an unsupported extension. The only suppo
!!! error TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.
import {} from "./b.d.ts";
~~~~~~~~~~
!!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './b' instead?
!!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './b.js' instead?
import type {} from "./b.d.ts";

import {} from "./c.ts";
Expand Down Expand Up @@ -96,5 +96,5 @@ error TS6054: File '/project/e.txt' has an unsupported extension. The only suppo
import {} from "./a.ts";
import {} from "./a.d.ts";
~~~~~~~~~~
!!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a' instead?
!!! error TS2846: A declaration file cannot be imported without 'import type'. Did you mean to import an implementation file './a.js' instead?
import type {} from "./a.d.ts";
Original file line number Diff line number Diff line change
Expand Up @@ -66,17 +66,12 @@ import {} from "./a.d.ts";
import type {} from "./a.d.ts";

//// [a.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
//// [index.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
//// [e.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
//// [e.txt.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
//// [main.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
export {};
Loading