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

Simplify extension priorities and move dts to lowest priority #34713

Closed
wants to merge 4 commits into from
Closed

Simplify extension priorities and move dts to lowest priority #34713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
55343fb
Simplify extension priorities, handle custom extensions better (ish),…
weswigham Oct 24, 2019
55d5c47
Merge branch 'master' into extension-priority-revamp
weswigham Feb 24, 2020
12debca
Merge branch 'master' into extension-priority-revamp
weswigham Mar 10, 2020
415500c
Merge branch 'extension-priority-revamp' of github.com:weswigham/Type…
weswigham Mar 10, 2020
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: 7 additions & 6 deletions src/compiler/commandLineParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3139,9 +3139,8 @@ namespace ts {
* @param context The expansion context.
*/
function hasFileWithHigherPriorityExtension(file: string, literalFiles: Map<string>, wildcardFiles: Map<string>, extensions: readonly string[], keyMapper: (value: string) => string) {
const extensionPriority = getExtensionPriority(file, extensions);
const adjustedExtensionPriority = adjustExtensionPriority(extensionPriority, extensions);
for (let i = ExtensionPriority.Highest; i < adjustedExtensionPriority; i++) {
const extensionPriority = extensions.indexOf(tryGetExtensionFromPath(file)!); // unknown extension is -1
for (let i = 0; i < extensionPriority; i++) {
const higherPriorityExtension = extensions[i];
const higherPriorityPath = keyMapper(changeExtension(file, higherPriorityExtension));
if (literalFiles.has(higherPriorityPath) || wildcardFiles.has(higherPriorityPath)) {
Expand All @@ -3161,9 +3160,11 @@ namespace ts {
* @param context The expansion context.
*/
function removeWildcardFilesWithLowerPriorityExtension(file: string, wildcardFiles: Map<string>, extensions: readonly string[], keyMapper: (value: string) => string) {
const extensionPriority = getExtensionPriority(file, extensions);
const nextExtensionPriority = getNextLowestExtensionPriority(extensionPriority, extensions);
for (let i = nextExtensionPriority; i < extensions.length; i++) {
const extensionPriority = extensions.indexOf(tryGetExtensionFromPath(file)!); // unknown extension is -1
if (extensionPriority === -1) {
return;
}
for (let i = extensionPriority; i < extensions.length; i++) {
const lowerPriorityExtension = extensions[i];
const lowerPriorityPath = keyMapper(changeExtension(file, lowerPriorityExtension));
wildcardFiles.delete(lowerPriorityPath);
Expand Down
60 changes: 4 additions & 56 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5849,13 +5849,13 @@ namespace ts {
* List of supported extensions in order of file resolution precedence.
*/
export const supportedTSExtensions: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Dts];
export const supportedTSExtensionsWithJson: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Dts, Extension.Json];
export const supportedTSExtensionsWithJson: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Json, Extension.Dts];
Copy link
Member

Choose a reason for hiding this comment

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

This isn't ideal I think...

Consider scenario::

// moduleA.ts
import { x } from "./moduleB";

// moduleB.js
export const x = 10;

When you build first time files in your program with allowJs and emitDeclarationsOnly will be:
moduleA.ts, moduleB.js
Next time they will be
moduleA.ts, moduleB.d.ts and moduleB.js since module resolution prefers up d.ts over .js

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what we do today. This PR fixes that, such that the .d.ts is never preferred if the js is present, assuming allowJs is on.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is rootFiles will be moduleA.ts and moduleB.js but final list of files will contain .d.ts since that's what module resolution will resolve to? https://github.com/microsoft/TypeScript/blob/master/src/compiler/moduleNameResolver.ts#L1096

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, yeah, the order we check extensions in module resolution likely needs to match the order wildcards use. I'll look at changing it to use the same source, ideally.

/** Must have ".d.ts" first because if ".ts" goes first, that will be detected as the extension instead of ".d.ts". */
export const supportedTSExtensionsForExtractExtension: readonly Extension[] = [Extension.Dts, Extension.Ts, Extension.Tsx];
export const supportedJSExtensions: readonly Extension[] = [Extension.Js, Extension.Jsx];
export const supportedJSAndJsonExtensions: readonly Extension[] = [Extension.Js, Extension.Jsx, Extension.Json];
const allSupportedExtensions: readonly Extension[] = [...supportedTSExtensions, ...supportedJSExtensions];
const allSupportedExtensionsWithJson: readonly Extension[] = [...supportedTSExtensions, ...supportedJSExtensions, Extension.Json];
const allSupportedExtensions: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Js, Extension.Jsx, Extension.Dts];
const allSupportedExtensionsWithJson: readonly Extension[] = [Extension.Ts, Extension.Tsx, Extension.Js, Extension.Jsx, Extension.Json, Extension.Dts];

export function getSupportedExtensions(options?: CompilerOptions): readonly Extension[];
export function getSupportedExtensions(options?: CompilerOptions, extraFileExtensions?: readonly FileExtensionInfo[]): readonly string[];
Expand All @@ -5878,7 +5878,7 @@ namespace ts {
if (!options || !options.resolveJsonModule) { return supportedExtensions; }
if (supportedExtensions === allSupportedExtensions) { return allSupportedExtensionsWithJson; }
if (supportedExtensions === supportedTSExtensions) { return supportedTSExtensionsWithJson; }
return [...supportedExtensions, Extension.Json];
return [...supportedExtensions, Extension.Json]; // TODO: Insert Json before DTS (q: when is this codepath hit?)
}

function isJSLike(scriptKind: ScriptKind | undefined): boolean {
Expand All @@ -5905,58 +5905,6 @@ namespace ts {
return false;
}

/**
* Extension boundaries by priority. Lower numbers indicate higher priorities, and are
* aligned to the offset of the highest priority extension in the
* allSupportedExtensions array.
*/
export const enum ExtensionPriority {
TypeScriptFiles = 0,
DeclarationAndJavaScriptFiles = 2,

Highest = TypeScriptFiles,
Lowest = DeclarationAndJavaScriptFiles,
}

export function getExtensionPriority(path: string, supportedExtensions: readonly string[]): ExtensionPriority {
for (let i = supportedExtensions.length - 1; i >= 0; i--) {
if (fileExtensionIs(path, supportedExtensions[i])) {
return adjustExtensionPriority(<ExtensionPriority>i, supportedExtensions);
}
}

// If its not in the list of supported extensions, this is likely a
// TypeScript file with a non-ts extension
return ExtensionPriority.Highest;
}

/**
* Adjusts an extension priority to be the highest priority within the same range.
*/
export function adjustExtensionPriority(extensionPriority: ExtensionPriority, supportedExtensions: readonly string[]): ExtensionPriority {
if (extensionPriority < ExtensionPriority.DeclarationAndJavaScriptFiles) {
return ExtensionPriority.TypeScriptFiles;
}
else if (extensionPriority < supportedExtensions.length) {
return ExtensionPriority.DeclarationAndJavaScriptFiles;
}
else {
return supportedExtensions.length;
}
}

/**
* Gets the next lowest extension priority for a given priority.
*/
export function getNextLowestExtensionPriority(extensionPriority: ExtensionPriority, supportedExtensions: readonly string[]): ExtensionPriority {
if (extensionPriority < ExtensionPriority.DeclarationAndJavaScriptFiles) {
return ExtensionPriority.DeclarationAndJavaScriptFiles;
}
else {
return supportedExtensions.length;
}
}

const extensionsToRemove = [Extension.Dts, Extension.Ts, Extension.Js, Extension.Tsx, Extension.Jsx, Extension.Json];
export function removeFileExtension(path: string): string {
for (const ext of extensionsToRemove) {
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/jsFileCompilationWithMapFileAsJs.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error TS5055: Cannot write file 'tests/cases/compiler/b.js' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.d.ts', '.js', '.jsx'.
error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.js', '.jsx', '.d.ts'.


!!! error TS5055: Cannot write file 'tests/cases/compiler/b.js' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
!!! error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.d.ts', '.js', '.jsx'.
!!! error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.js', '.jsx', '.d.ts'.
==== tests/cases/compiler/a.ts (0 errors) ====
class c {
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/jsFileCompilationWithMapFileAsJsWithInlineSourceMap.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error TS5055: Cannot write file 'tests/cases/compiler/b.js' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.d.ts', '.js', '.jsx'.
error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.js', '.jsx', '.d.ts'.


!!! error TS5055: Cannot write file 'tests/cases/compiler/b.js' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
!!! error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.d.ts', '.js', '.jsx'.
!!! error TS6054: File 'tests/cases/compiler/b.js.map' has an unsupported extension. The only supported extensions are '.ts', '.tsx', '.js', '.jsx', '.d.ts'.
==== tests/cases/compiler/a.ts (0 errors) ====
class c {
}
Expand Down
1 change: 1 addition & 0 deletions ...mpilationSameNameDtsNotSpecifiedWithAllowJs/amd/SameNameDTsNotSpecifiedWithAllowJs/a.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
declare var test1: number;
6 changes: 0 additions & 6 deletions ...otSpecifiedWithAllowJs/amd/jsFileCompilationSameNameDtsNotSpecifiedWithAllowJs.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.d.ts' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.js' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.


!!! error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.d.ts' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
!!! error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.js' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
==== SameNameDTsNotSpecifiedWithAllowJs/tsconfig.json (0 errors) ====
{ "compilerOptions": { "allowJs": true } }
==== SameNameDTsNotSpecifiedWithAllowJs/a.d.ts (0 errors) ====
declare var a: number;
==== SameNameDTsNotSpecifiedWithAllowJs/a.js (0 errors) ====
var test1 = 10; // Shouldnt get compiled
5 changes: 3 additions & 2 deletions ...meDtsNotSpecifiedWithAllowJs/amd/jsFileCompilationSameNameDtsNotSpecifiedWithAllowJs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
"project": "SameNameDTsNotSpecifiedWithAllowJs",
"resolvedInputFiles": [
"lib.es5.d.ts",
"SameNameDTsNotSpecifiedWithAllowJs/a.d.ts",
"SameNameDTsNotSpecifiedWithAllowJs/a.js"
],
"emittedFiles": []
"emittedFiles": [
"SameNameDTsNotSpecifiedWithAllowJs/a.d.ts"
]
}
1 change: 1 addition & 0 deletions ...pilationSameNameDtsNotSpecifiedWithAllowJs/node/SameNameDTsNotSpecifiedWithAllowJs/a.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
declare var test1: number;
6 changes: 0 additions & 6 deletions ...tSpecifiedWithAllowJs/node/jsFileCompilationSameNameDtsNotSpecifiedWithAllowJs.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.d.ts' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.js' because it would overwrite input file.
Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.


!!! error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.d.ts' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
!!! error TS5055: Cannot write file 'SameNameDTsNotSpecifiedWithAllowJs/a.js' because it would overwrite input file.
!!! error TS5055: Adding a tsconfig.json file will help organize projects that contain both TypeScript and JavaScript files. Learn more at https://aka.ms/tsconfig.
==== SameNameDTsNotSpecifiedWithAllowJs/tsconfig.json (0 errors) ====
{ "compilerOptions": { "allowJs": true } }
==== SameNameDTsNotSpecifiedWithAllowJs/a.d.ts (0 errors) ====
declare var a: number;
==== SameNameDTsNotSpecifiedWithAllowJs/a.js (0 errors) ====
var test1 = 10; // Shouldnt get compiled
5 changes: 3 additions & 2 deletions ...eDtsNotSpecifiedWithAllowJs/node/jsFileCompilationSameNameDtsNotSpecifiedWithAllowJs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
"project": "SameNameDTsNotSpecifiedWithAllowJs",
"resolvedInputFiles": [
"lib.es5.d.ts",
"SameNameDTsNotSpecifiedWithAllowJs/a.d.ts",
"SameNameDTsNotSpecifiedWithAllowJs/a.js"
],
"emittedFiles": []
"emittedFiles": [
"SameNameDTsNotSpecifiedWithAllowJs/a.d.ts"
]
}