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

Jump to bottom

moduleDetection: auto makes cjs/cts files parse as modules, and ... #49268

Merged
merged 1 commit into from May 26, 2022
Merged

moduleDetection: auto makes cjs/cts files parse as modules, and ... #49268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/compiler/binder.ts
View file
Open in desktop
Expand Up @@ -2834,12 +2834,14 @@ namespace ts {
}

function setCommonJsModuleIndicator(node: Node) {
if (file.externalModuleIndicator) {
if (file.externalModuleIndicator && file.externalModuleIndicator !== true) {
return false;
}
if (!file.commonJsModuleIndicator) {
file.commonJsModuleIndicator = node;
bindSourceFileAsExternalModule();
if (!file.externalModuleIndicator) {
bindSourceFileAsExternalModule();
}
}
return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/checker.ts
View file
Open in desktop
Expand Up @@ -2746,7 +2746,7 @@ namespace ts {
return hasExportAssignmentSymbol(moduleSymbol);
}
// JS files have a synthetic default if they do not contain ES2015+ module syntax (export = is not valid in js) _and_ do not have an __esModule marker
return !file.externalModuleIndicator && !resolveExportByName(moduleSymbol, escapeLeadingUnderscores("__esModule"), /*sourceNode*/ undefined, dontResolveAlias);
return typeof file.externalModuleIndicator !== "object" && !resolveExportByName(moduleSymbol, escapeLeadingUnderscores("__esModule"), /*sourceNode*/ undefined, dontResolveAlias);
}

function getTargetOfImportClause(node: ImportClause, dontResolveAlias: boolean): Symbol | undefined {
Expand Down
10 changes: 6 additions & 4 deletions src/compiler/utilities.ts
View file
Open in desktop
Expand Up @@ -6305,8 +6305,9 @@ namespace ts {
*/
function isFileForcedToBeModuleByFormat(file: SourceFile): true | undefined {
// Excludes declaration files - they still require an explicit `export {}` or the like
// for back compat purposes.
return file.impliedNodeFormat === ModuleKind.ESNext && !file.isDeclarationFile ? true : undefined;
// for back compat purposes. The only non-declaration files _not_ forced to be a module are `.js` files
// that aren't esm-mode (meaning not in a `type: module` scope).
return (file.impliedNodeFormat === ModuleKind.ESNext || (fileExtensionIsOneOf(file.fileName, [Extension.Cjs, Extension.Cts]))) && !file.isDeclarationFile ? true : undefined;
}

export function getSetExternalModuleIndicator(options: CompilerOptions): (file: SourceFile) => void {
Expand All @@ -6315,7 +6316,7 @@ namespace ts {
case ModuleDetectionKind.Force:
// All non-declaration files are modules, declaration files still do the usual isFileProbablyExternalModule
return (file: SourceFile) => {
file.externalModuleIndicator = !file.isDeclarationFile || isFileProbablyExternalModule(file);
file.externalModuleIndicator = isFileProbablyExternalModule(file) || !file.isDeclarationFile || undefined;
};
case ModuleDetectionKind.Legacy:
// Files are modules if they have imports, exports, or import.meta
Expand Down Expand Up @@ -6375,7 +6376,8 @@ namespace ts {
}

export function getEmitModuleDetectionKind(options: CompilerOptions) {
return options.moduleDetection || ModuleDetectionKind.Auto;
return options.moduleDetection ||
(getEmitModuleKind(options) === ModuleKind.Node16 || getEmitModuleKind(options) === ModuleKind.NodeNext ? ModuleDetectionKind.Force : ModuleDetectionKind.Auto);
}

export function hasJsonModuleEmitEnabled(options: CompilerOptions) {
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/moduleResolutionWithoutExtension8.js
View file
Open in desktop
Expand Up @@ -3,5 +3,7 @@
import("./foo").then(x => x); // should error, ask for extension

//// [bar.cjs]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
// Extensionless relative path dynamic import in a cjs module
import("./foo").then(x => x); // should error, ask for extension
2 changes: 2 additions & 0 deletions tests/baselines/reference/nodeModulesAllowJsExportAssignment(module=node16).js
View file
Open in desktop
Expand Up @@ -34,6 +34,8 @@ module.exports = a;
const a = {};
module.exports = a;
//// [file.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
// cjs format file
const a = {};
module.exports = a;
Expand Down
2 changes: 2 additions & 0 deletions tests/baselines/reference/nodeModulesAllowJsExportAssignment(module=nodenext).js
View file
Open in desktop
Expand Up @@ -34,6 +34,8 @@ module.exports = a;
const a = {};
module.exports = a;
//// [file.js]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
// cjs format file
const a = {};
module.exports = a;
Expand Down
13 changes: 13 additions & 0 deletions ...s/baselines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=node16).js
View file
Open in desktop
@@ -0,0 +1,13 @@
//// [tests/cases/conformance/node/allowJs/nodeModulesAllowJsExportlessJsModuleDetectionAuto.ts] ////

//// [foo.cjs]
// this file is a module despite having no imports
//// [bar.js]
// however this file is _not_ a module

//// [foo.cjs]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
// this file is a module despite having no imports
//// [bar.js]
// however this file is _not_ a module
5 changes: 5 additions & 0 deletions ...elines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=node16).symbols
View file
Open in desktop
@@ -0,0 +1,5 @@
=== tests/cases/conformance/node/allowJs/foo.cjs ===
// this file is a module despite having no imports
No type information for this code.=== tests/cases/conformance/node/allowJs/bar.js ===
// however this file is _not_ a module
No type information for this code.
5 changes: 5 additions & 0 deletions ...aselines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=node16).types
View file
Open in desktop
@@ -0,0 +1,5 @@
=== tests/cases/conformance/node/allowJs/foo.cjs ===
// this file is a module despite having no imports
No type information for this code.=== tests/cases/conformance/node/allowJs/bar.js ===
// however this file is _not_ a module
No type information for this code.
13 changes: 13 additions & 0 deletions ...baselines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=nodenext).js
View file
Open in desktop
@@ -0,0 +1,13 @@
//// [tests/cases/conformance/node/allowJs/nodeModulesAllowJsExportlessJsModuleDetectionAuto.ts] ////

//// [foo.cjs]
// this file is a module despite having no imports
//// [bar.js]
// however this file is _not_ a module

//// [foo.cjs]
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
// this file is a module despite having no imports
//// [bar.js]
// however this file is _not_ a module
5 changes: 5 additions & 0 deletions ...ines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=nodenext).symbols
View file
Open in desktop
@@ -0,0 +1,5 @@
=== tests/cases/conformance/node/allowJs/foo.cjs ===
// this file is a module despite having no imports
No type information for this code.=== tests/cases/conformance/node/allowJs/bar.js ===
// however this file is _not_ a module
No type information for this code.
5 changes: 5 additions & 0 deletions ...elines/reference/nodeModulesAllowJsExportlessJsModuleDetectionAuto(module=nodenext).types
View file
Open in desktop
@@ -0,0 +1,5 @@
=== tests/cases/conformance/node/allowJs/foo.cjs ===
// this file is a module despite having no imports
No type information for this code.=== tests/cases/conformance/node/allowJs/bar.js ===
// however this file is _not_ a module
No type information for this code.
2 changes: 2 additions & 0 deletions tests/baselines/reference/tripleSlashTypesReferenceWithMissingExports(module=node16).js
View file
Open in desktop
Expand Up @@ -14,5 +14,7 @@ interface GlobalThing { a: number }
const a: GlobalThing = { a: 0 };

//// [usage.js]
"use strict";
/// <reference types="pkg" />
Object.defineProperty(exports, "__esModule", { value: true });
const a = { a: 0 };
2 changes: 2 additions & 0 deletions tests/baselines/reference/tripleSlashTypesReferenceWithMissingExports(module=nodenext).js
View file
Open in desktop
Expand Up @@ -14,5 +14,7 @@ interface GlobalThing { a: number }
const a: GlobalThing = { a: 0 };

//// [usage.js]
"use strict";
/// <reference types="pkg" />
Object.defineProperty(exports, "__esModule", { value: true });
const a = { a: 0 };
8 changes: 8 additions & 0 deletions tests/cases/conformance/node/allowJs/nodeModulesAllowJsExportlessJsModuleDetectionAuto.ts
View file
Open in desktop
@@ -0,0 +1,8 @@
// @module: node16,nodenext
// @allowJs: true
// @outDir: ./out
// @moduleDetection: auto
// @filename: foo.cjs
Copy link
Member

Choose a reason for hiding this comment

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

Does this also need change to isExternalOrCommonJsModule i think during incremental buildInfo doesnt mark bar.js as affecting global scope and that is a bug ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik that just reads the two marker fields, which are all that get set to determine module-ness.

Copy link
Member

Choose a reason for hiding this comment

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

can you add test and ensure that bar.js will be marked as affects global scope and foo.cjs as module so no affectsGlobalScope in buildInfo. I am pretty certain that it is broken right now.

// this file is a module despite having no imports
// @filename: bar.js
// however this file is _not_ a module