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

For duplicate source files of the same package, make one redirect to the other #16274

Merged
19 commits merged into from
Aug 9, 2017
Merged

For duplicate source files of the same package, make one redirect to the other #16274

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
60b9aa3
For duplicate source files of the same package, make one redirect to …
May 31, 2017
8157308
Add reuseProgramStructure tests
Jun 7, 2017
e4d5f0d
Copy `sourceFileToPackageId` and `isSourceFileTargetOfRedirect` only …
Jun 7, 2017
17c5619
Merge branch 'master' into package-id
Jun 7, 2017
1f32e62
Use fallthrough instead of early exit from loop
Jun 7, 2017
f66713d
Use a set to efficiently detect duplicate package names
Jun 7, 2017
04cae2e
Move map setting outside of createRedirectSourceFile
Jun 7, 2017
61b768a
Correctly handle seenPackageNames set
Jun 7, 2017
4d55f34
sourceFileToPackageId -> sourceFileToPackageName
Jun 7, 2017
d702180
Merge branch 'master' into package-id
Jun 7, 2017
e2a0931
Merge branch 'master' into package-id
Jul 10, 2017
09e1c92
Renames
Jul 13, 2017
d557e9a
Merge branch 'master' into package-id
Jul 13, 2017
3d2fc00
Respond to PR comments
Jul 14, 2017
018c9c5
Fix bug where `oldSourceFile !== newSourceFile` because oldSourceFile…
Jul 14, 2017
91b2575
Clean up redirectInfo
Aug 3, 2017
bec2d03
Merge branch 'master' into package-id
Aug 3, 2017
8408b3d
Respond to PR comments
Aug 9, 2017
9f74f55
Merge branch 'master' into package-id
Aug 9, 2017
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
18 changes: 9 additions & 9 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2164,20 +2164,15 @@ namespace ts {
/** Must have ".d.ts" first because if ".ts" goes first, that will be detected as the extension instead of ".d.ts". */
export const supportedTypescriptExtensionsForExtractExtension: ReadonlyArray<Extension> = [Extension.Dts, Extension.Ts, Extension.Tsx];
export const supportedJavascriptExtensions: ReadonlyArray<Extension> = [Extension.Js, Extension.Jsx];
const allSupportedExtensions = [...supportedTypeScriptExtensions, ...supportedJavascriptExtensions];
const allSupportedExtensions: ReadonlyArray<Extension> = [...supportedTypeScriptExtensions, ...supportedJavascriptExtensions];

export function getSupportedExtensions(options?: CompilerOptions, extraFileExtensions?: ReadonlyArray<JsFileExtensionInfo>): ReadonlyArray<string> {
const needAllExtensions = options && options.allowJs;
if (!extraFileExtensions || extraFileExtensions.length === 0 || !needAllExtensions) {
return needAllExtensions ? allSupportedExtensions : supportedTypeScriptExtensions;
// TODO: Return a ReadonlyArray<string> from this function to avoid casts. https://github.com/Microsoft/TypeScript/issues/16312
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Return a ReadonlyArray<string> to avoid casts. see https://..."

return needAllExtensions ? allSupportedExtensions as Extension[] : supportedTypeScriptExtensions as Extension[];
}
const extensions: string[] = allSupportedExtensions.slice(0);
for (const extInfo of extraFileExtensions) {
if (extensions.indexOf(extInfo.extension) === -1) {
extensions.push(extInfo.extension);
}
}
return extensions;
return deduplicate([...allSupportedExtensions, ...extraFileExtensions.map(e => e.extension)]);
Copy link
Member

Choose a reason for hiding this comment

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

For my own edification, does this have some advantage over calling concat?

Copy link
Author

Choose a reason for hiding this comment

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

See #17076

Copy link
Member

Choose a reason for hiding this comment

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

Cute.

}

export function hasJavaScriptFileExtension(fileName: string) {
Expand Down Expand Up @@ -2519,6 +2514,11 @@ namespace ts {
}
Debug.fail(`File ${path} has unknown extension.`);
}

export function isAnySupportedFileExtension(path: string): boolean {
return tryGetExtensionFromPath(path) !== undefined;
}

export function tryGetExtensionFromPath(path: string): Extension | undefined {
return find<Extension>(supportedTypescriptExtensionsForExtractExtension, e => fileExtensionIs(path, e)) || find(supportedJavascriptExtensions, e => fileExtensionIs(path, e));
}
Expand Down
97 changes: 63 additions & 34 deletions src/compiler/moduleNameResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,26 @@ namespace ts {
push(value: T): void;
}

/**
* Result of trying to resolve a module.
* At least one of `ts` and `js` should be defined, or the whole thing should be `undefined`.
*/
function withPackageId(packageId: PackageId | undefined, r: PathAndExtension | undefined): Resolved {
return r && { path: r.path, extension: r.ext, packageId };
}

function noPackageId(r: PathAndExtension | undefined): Resolved {
return withPackageId(/*packageId*/ undefined, r);
}

/** Result of trying to resolve a module. */
interface Resolved {
path: string;
extension: Extension;
packageId: PackageId | undefined;
}

/** Result of trying to resolve a module at a file. Needs to have 'packageId' added later. */
interface PathAndExtension {
path: string;
// (Use a different name than `extension` to make sure Resolved isn't assignable to PathAndExtension.)
Copy link
Member

Choose a reason for hiding this comment

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

Is it important that they not be assignable to each other? Is it just to avoid confusion or something more?

Copy link
Author

@ghost ghost Jul 14, 2017 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

This prevents us from computing packageId and then not using it. See #12936

ext: Extension;
}

/**
Expand All @@ -49,7 +62,7 @@ namespace ts {

function createResolvedModuleWithFailedLookupLocations(resolved: Resolved | undefined, isExternalLibraryImport: boolean, failedLookupLocations: string[]): ResolvedModuleWithFailedLookupLocations {
return {
resolvedModule: resolved && { resolvedFileName: resolved.path, extension: resolved.extension, isExternalLibraryImport },
resolvedModule: resolved && { resolvedFileName: resolved.path, extension: resolved.extension, isExternalLibraryImport, packageId: resolved.packageId },
failedLookupLocations
};
}
Expand All @@ -65,8 +78,7 @@ namespace ts {
}

/** Reads from "main" or "types"/"typings" depending on `extensions`. */
function tryReadPackageJsonFields(readTypes: boolean, packageJsonPath: string, baseDirectory: string, state: ModuleResolutionState): string | undefined {
const jsonContent = readJson(packageJsonPath, state.host);
function tryReadPackageJsonFields(readTypes: boolean, jsonContent: PackageJson, baseDirectory: string, state: ModuleResolutionState): string | undefined {
return readTypes ? tryReadFromField("typings") || tryReadFromField("types") : tryReadFromField("main");

function tryReadFromField(fieldName: "typings" | "types" | "main"): string | undefined {
Expand All @@ -93,7 +105,8 @@ namespace ts {
}
}

function readJson(path: string, host: ModuleResolutionHost): { typings?: string, types?: string, main?: string } {
interface PackageJson { name?: string; version?: string; typings?: string; types?: string; main?: string; }
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is just an implementation detail (as it appears to be for me), then move this declaration up with the others at the top of this file. Otherwise, move it to types.ts.

Please add newlines.

function readJson(path: string, host: ModuleResolutionHost): PackageJson {
try {
const jsonText = host.readFile(path);
return jsonText ? JSON.parse(jsonText) : {};
Expand Down Expand Up @@ -654,7 +667,7 @@ namespace ts {
if (extension !== undefined) {
const path = tryFile(candidate, failedLookupLocations, /*onlyRecordFailures*/ false, state);
if (path !== undefined) {
return { path, extension };
return { path, extension, packageId: undefined };
}
}

Expand Down Expand Up @@ -717,7 +730,7 @@ namespace ts {
}
const resolved = loadModuleFromNodeModules(extensions, moduleName, containingDirectory, failedLookupLocations, state, cache);
// For node_modules lookups, get the real path so that multiple accesses to an `npm link`-ed module do not create duplicate files.
return resolved && { value: resolved.value && { resolved: { path: realpath(resolved.value.path, host, traceEnabled), extension: resolved.value.extension }, isExternalLibraryImport: true } };
return resolved && { value: resolved.value && { resolved: { ...resolved.value, path: realpath(resolved.value.path, host, traceEnabled) }, isExternalLibraryImport: true } };
}
else {
const candidate = normalizePath(combinePaths(containingDirectory, moduleName));
Expand Down Expand Up @@ -755,7 +768,7 @@ namespace ts {
}
const resolvedFromFile = loadModuleFromFile(extensions, candidate, failedLookupLocations, onlyRecordFailures, state);
if (resolvedFromFile) {
return resolvedFromFile;
return noPackageId(resolvedFromFile);
}
}
if (!onlyRecordFailures) {
Expand All @@ -776,11 +789,15 @@ namespace ts {
return !host.directoryExists || host.directoryExists(directoryName);
}

function loadModuleFromFileNoPackageId(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): Resolved {
return noPackageId(loadModuleFromFile(extensions, candidate, failedLookupLocations, onlyRecordFailures, state));
}

/**
* @param {boolean} onlyRecordFailures - if true then function won't try to actually load files but instead record all attempts as failures. This flag is necessary
* in cases when we know upfront that all load attempts will fail (because containing folder does not exists) however we still need to record all failed lookup locations.
*/
function loadModuleFromFile(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): Resolved | undefined {
function loadModuleFromFile(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): PathAndExtension | undefined {
// First, try adding an extension. An import of "foo" could be matched by a file "foo.ts", or "foo.js" by "foo.js.ts"
const resolvedByAddingExtension = tryAddingExtensions(candidate, extensions, failedLookupLocations, onlyRecordFailures, state);
if (resolvedByAddingExtension) {
Expand All @@ -800,7 +817,7 @@ namespace ts {
}

/** Try to return an existing file that adds one of the `extensions` to `candidate`. */
function tryAddingExtensions(candidate: string, extensions: Extensions, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): Resolved | undefined {
function tryAddingExtensions(candidate: string, extensions: Extensions, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState): PathAndExtension | undefined {
if (!onlyRecordFailures) {
// check if containing folder exists - if it doesn't then just record failures for all supported extensions without disk probing
const directory = getDirectoryPath(candidate);
Expand All @@ -818,9 +835,9 @@ namespace ts {
return tryExtension(Extension.Js) || tryExtension(Extension.Jsx);
}

function tryExtension(extension: Extension): Resolved | undefined {
const path = tryFile(candidate + extension, failedLookupLocations, onlyRecordFailures, state);
return path && { path, extension };
function tryExtension(ext: Extension): PathAndExtension | undefined {
const path = tryFile(candidate + ext, failedLookupLocations, onlyRecordFailures, state);
return path && { path, ext };
}
}

Expand All @@ -846,12 +863,23 @@ namespace ts {
function loadNodeModuleFromDirectory(extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, onlyRecordFailures: boolean, state: ModuleResolutionState, considerPackageJson = true): Resolved | undefined {
const directoryExists = !onlyRecordFailures && directoryProbablyExists(candidate, state.host);

let packageId: PackageId | undefined;

if (considerPackageJson) {
const packageJsonPath = pathToPackageJson(candidate);
if (directoryExists && state.host.fileExists(packageJsonPath)) {
const fromPackageJson = loadModuleFromPackageJson(packageJsonPath, extensions, candidate, failedLookupLocations, state);
if (state.traceEnabled) {
trace(state.host, Diagnostics.Found_package_json_at_0, packageJsonPath);
}
const jsonContent = readJson(packageJsonPath, state.host);

if (typeof jsonContent.name === "string" && typeof jsonContent.version === "string") {
packageId = { name: jsonContent.name, version: jsonContent.version };
}

const fromPackageJson = loadModuleFromPackageJson(jsonContent, extensions, candidate, failedLookupLocations, state);
if (fromPackageJson) {
return fromPackageJson;
return withPackageId(packageId, fromPackageJson);
}
}
else {
Expand All @@ -863,15 +891,11 @@ namespace ts {
}
}

return loadModuleFromFile(extensions, combinePaths(candidate, "index"), failedLookupLocations, !directoryExists, state);
return withPackageId(packageId, loadModuleFromFile(extensions, combinePaths(candidate, "index"), failedLookupLocations, !directoryExists, state));
}

function loadModuleFromPackageJson(packageJsonPath: string, extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, state: ModuleResolutionState): Resolved | undefined {
if (state.traceEnabled) {
trace(state.host, Diagnostics.Found_package_json_at_0, packageJsonPath);
}

const file = tryReadPackageJsonFields(extensions !== Extensions.JavaScript, packageJsonPath, candidate, state);
function loadModuleFromPackageJson(jsonContent: PackageJson, extensions: Extensions, candidate: string, failedLookupLocations: Push<string>, state: ModuleResolutionState): PathAndExtension | undefined {
const file = tryReadPackageJsonFields(extensions !== Extensions.JavaScript, jsonContent, candidate, state);
if (!file) {
return undefined;
}
Expand All @@ -891,13 +915,18 @@ namespace ts {
// Even if extensions is DtsOnly, we can still look up a .ts file as a result of package.json "types"
const nextExtensions = extensions === Extensions.DtsOnly ? Extensions.TypeScript : extensions;
// Don't do package.json lookup recursively, because Node.js' package lookup doesn't.
return nodeLoadModuleByRelativeName(nextExtensions, file, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ false);
const result = nodeLoadModuleByRelativeName(nextExtensions, file, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ false);
if (result) {
Copy link
Member

Choose a reason for hiding this comment

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

What gets returned otherwise? Is return undefined implied?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, JS functions always return undefined if they don't reach a return statement.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I like to be explicit, but I'm fine if this is idiomatic JS.

// It won't have a `packageId` set, because we disabled `considerPackageJson`.
Debug.assert(result.packageId === undefined);
return { path: result.path, ext: result.extension };
}
}

/** Resolve from an arbitrarily specified file. Return `undefined` if it has an unsupported extension. */
function resolvedIfExtensionMatches(extensions: Extensions, path: string): Resolved | undefined {
const extension = tryGetExtensionFromPath(path);
return extension !== undefined && extensionIsOk(extensions, extension) ? { path, extension } : undefined;
function resolvedIfExtensionMatches(extensions: Extensions, path: string): PathAndExtension | undefined {
const ext = tryGetExtensionFromPath(path);
return ext !== undefined && extensionIsOk(extensions, ext) ? { path, ext } : undefined;
}

/** True if `extension` is one of the supported `extensions`. */
Expand All @@ -919,7 +948,7 @@ namespace ts {
function loadModuleFromNodeModulesFolder(extensions: Extensions, moduleName: string, nodeModulesFolder: string, nodeModulesFolderExists: boolean, failedLookupLocations: Push<string>, state: ModuleResolutionState): Resolved | undefined {
const candidate = normalizePath(combinePaths(nodeModulesFolder, moduleName));

return loadModuleFromFile(extensions, candidate, failedLookupLocations, !nodeModulesFolderExists, state) ||
return loadModuleFromFileNoPackageId(extensions, candidate, failedLookupLocations, !nodeModulesFolderExists, state) ||
loadNodeModuleFromDirectory(extensions, candidate, failedLookupLocations, !nodeModulesFolderExists, state);
}

Expand Down Expand Up @@ -1004,7 +1033,7 @@ namespace ts {
if (traceEnabled) {
trace(host, Diagnostics.Resolution_for_module_0_was_found_in_cache, moduleName);
}
return { value: result.resolvedModule && { path: result.resolvedModule.resolvedFileName, extension: result.resolvedModule.extension } };
return { value: result.resolvedModule && { path: result.resolvedModule.resolvedFileName, extension: result.resolvedModule.extension, packageId: result.resolvedModule.packageId } };
}
}

Expand All @@ -1018,7 +1047,7 @@ namespace ts {
return createResolvedModuleWithFailedLookupLocations(resolved && resolved.value, /*isExternalLibraryImport*/ false, failedLookupLocations);

function tryResolve(extensions: Extensions): SearchResult<Resolved> {
const resolvedUsingSettings = tryLoadModuleUsingOptionalResolutionSettings(extensions, moduleName, containingDirectory, loadModuleFromFile, failedLookupLocations, state);
const resolvedUsingSettings = tryLoadModuleUsingOptionalResolutionSettings(extensions, moduleName, containingDirectory, loadModuleFromFileNoPackageId, failedLookupLocations, state);
if (resolvedUsingSettings) {
return { value: resolvedUsingSettings };
}
Expand All @@ -1032,7 +1061,7 @@ namespace ts {
return resolutionFromCache;
}
const searchName = normalizePath(combinePaths(directory, moduleName));
return toSearchResult(loadModuleFromFile(extensions, searchName, failedLookupLocations, /*onlyRecordFailures*/ false, state));
return toSearchResult(loadModuleFromFileNoPackageId(extensions, searchName, failedLookupLocations, /*onlyRecordFailures*/ false, state));
});
if (resolved) {
return resolved;
Expand All @@ -1044,7 +1073,7 @@ namespace ts {
}
else {
const candidate = normalizePath(combinePaths(containingDirectory, moduleName));
return toSearchResult(loadModuleFromFile(extensions, candidate, failedLookupLocations, /*onlyRecordFailures*/ false, state));
return toSearchResult(loadModuleFromFileNoPackageId(extensions, candidate, failedLookupLocations, /*onlyRecordFailures*/ false, state));
}
}
}
Expand Down
Loading