From 82d4cccf5360bc073c9aba49ea8c83714835bea5 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Fri, 10 Aug 2018 15:23:35 -0700 Subject: [PATCH] Use original path from resolved module cache when available. Fixes #26273 Thanks to @ajafff, for partial fix and test (#26310) --- src/compiler/moduleNameResolver.ts | 29 +++++++++---------- src/testRunner/unittests/moduleResolution.ts | 17 +++++++++++ .../unittests/tsserverProjectSystem.ts | 2 +- .../cachedModuleResolution1.trace.json | 1 - .../cachedModuleResolution2.trace.json | 1 - .../cachedModuleResolution5.trace.json | 1 - 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts index d973be251c5..9c01238bc51 100644 --- a/src/compiler/moduleNameResolver.ts +++ b/src/compiler/moduleNameResolver.ts @@ -29,6 +29,7 @@ namespace ts { path: string; extension: Extension; packageId: PackageId | undefined; + originalPath?: string | true; } /** Result of trying to resolve a module at a file. Needs to have 'packageId' added later. */ @@ -62,9 +63,9 @@ namespace ts { return { fileName: resolved.path, packageId: resolved.packageId }; } - function createResolvedModuleWithFailedLookupLocations(resolved: Resolved | undefined, originalPath: string | undefined, isExternalLibraryImport: boolean, failedLookupLocations: string[]): ResolvedModuleWithFailedLookupLocations { + function createResolvedModuleWithFailedLookupLocations(resolved: Resolved | undefined, isExternalLibraryImport: boolean, failedLookupLocations: string[]): ResolvedModuleWithFailedLookupLocations { return { - resolvedModule: resolved && { resolvedFileName: resolved.path, originalPath, extension: resolved.extension, isExternalLibraryImport, packageId: resolved.packageId }, + resolvedModule: resolved && { resolvedFileName: resolved.path, originalPath: resolved.originalPath === true ? undefined : resolved.originalPath, extension: resolved.extension, isExternalLibraryImport, packageId: resolved.packageId }, failedLookupLocations }; } @@ -754,12 +755,12 @@ namespace ts { tryResolve(Extensions.JavaScript) || (compilerOptions.resolveJsonModule ? tryResolve(Extensions.Json) : undefined)); if (result && result.value) { - const { resolved, originalPath, isExternalLibraryImport } = result.value; - return createResolvedModuleWithFailedLookupLocations(resolved, originalPath, isExternalLibraryImport, failedLookupLocations); + const { resolved, isExternalLibraryImport } = result.value; + return createResolvedModuleWithFailedLookupLocations(resolved, isExternalLibraryImport, failedLookupLocations); } return { resolvedModule: undefined, failedLookupLocations }; - function tryResolve(extensions: Extensions): SearchResult<{ resolved: Resolved, originalPath?: string, isExternalLibraryImport: boolean }> { + function tryResolve(extensions: Extensions): SearchResult<{ resolved: Resolved, isExternalLibraryImport: boolean }> { const loader: ResolutionKindSpecificLoader = (extensions, candidate, failedLookupLocations, onlyRecordFailures, state) => nodeLoadModuleByRelativeName(extensions, candidate, failedLookupLocations, onlyRecordFailures, state, /*considerPackageJson*/ true); const resolved = tryLoadModuleUsingOptionalResolutionSettings(extensions, moduleName, containingDirectory, loader, failedLookupLocations, state); if (resolved) { @@ -774,17 +775,13 @@ namespace ts { if (!resolved) return undefined; let resolvedValue = resolved.value; - let originalPath: string | undefined; - if (!compilerOptions.preserveSymlinks && resolvedValue) { - originalPath = resolvedValue.path; + if (!compilerOptions.preserveSymlinks && resolvedValue && !resolvedValue.originalPath) { const path = realPath(resolvedValue.path, host, traceEnabled); - if (path === originalPath) { - originalPath = undefined; - } - resolvedValue = { ...resolvedValue, path }; + const originalPath = path === resolvedValue.path ? undefined : resolvedValue.path; + resolvedValue = { ...resolvedValue, path, originalPath }; } // For node_modules lookups, get the real path so that multiple accesses to an `npm link`-ed module do not create duplicate files. - return { value: resolvedValue && { resolved: resolvedValue, originalPath, isExternalLibraryImport: true } }; + return { value: resolvedValue && { resolved: resolvedValue, isExternalLibraryImport: true } }; } else { const { path: candidate, parts } = normalizePathAndParts(combinePaths(containingDirectory, moduleName)); @@ -1234,7 +1231,7 @@ namespace ts { trace(host, Diagnostics.Resolution_for_module_0_was_found_in_cache_from_location_1, moduleName, containingDirectory); } failedLookupLocations.push(...result.failedLookupLocations); - return { value: result.resolvedModule && { path: result.resolvedModule.resolvedFileName, extension: result.resolvedModule.extension, packageId: result.resolvedModule.packageId } }; + return { value: result.resolvedModule && { path: result.resolvedModule.resolvedFileName, originalPath: result.resolvedModule.originalPath || true, extension: result.resolvedModule.extension, packageId: result.resolvedModule.packageId } }; } } @@ -1246,7 +1243,7 @@ namespace ts { const resolved = tryResolve(Extensions.TypeScript) || tryResolve(Extensions.JavaScript); // No originalPath because classic resolution doesn't resolve realPath - return createResolvedModuleWithFailedLookupLocations(resolved && resolved.value, /*originalPath*/ undefined, /*isExternalLibraryImport*/ false, failedLookupLocations); + return createResolvedModuleWithFailedLookupLocations(resolved && resolved.value, /*isExternalLibraryImport*/ false, failedLookupLocations); function tryResolve(extensions: Extensions): SearchResult { const resolvedUsingSettings = tryLoadModuleUsingOptionalResolutionSettings(extensions, moduleName, containingDirectory, loadModuleFromFileNoPackageId, failedLookupLocations, state); @@ -1293,7 +1290,7 @@ namespace ts { const state: ModuleResolutionState = { compilerOptions, host, traceEnabled }; const failedLookupLocations: string[] = []; const resolved = loadModuleFromNodeModulesOneLevel(Extensions.DtsOnly, moduleName, globalCache, failedLookupLocations, state); - return createResolvedModuleWithFailedLookupLocations(resolved, /*originalPath*/ undefined, /*isExternalLibraryImport*/ true, failedLookupLocations); + return createResolvedModuleWithFailedLookupLocations(resolved, /*isExternalLibraryImport*/ true, failedLookupLocations); } /** diff --git a/src/testRunner/unittests/moduleResolution.ts b/src/testRunner/unittests/moduleResolution.ts index 76afb4a9b82..0b2c53a0ec2 100644 --- a/src/testRunner/unittests/moduleResolution.ts +++ b/src/testRunner/unittests/moduleResolution.ts @@ -427,6 +427,23 @@ namespace ts { resolution = resolveModuleName("a", "/foo.ts", compilerOptions, host, cache); assert.isUndefined(resolution.resolvedModule, "lookup in parent directory doesn't hit the cache"); }); + + it("preserves originalPath on cache hit", () => { + const host = createModuleResolutionHost( + /*hasDirectoryExists*/ true, + { name: "/linked/index.d.ts", symlinks: ["/app/node_modules/linked/index.d.ts"] }, + { name: "/app/node_modules/linked/package.json", content: '{"version": "0.0.0", "main": "./index"}' }, + ); + const cache = createModuleResolutionCache("/", (f) => f); + const compilerOptions: CompilerOptions = { moduleResolution: ModuleResolutionKind.NodeJs }; + checkResolution(resolveModuleName("linked", "/app/src/app.ts", compilerOptions, host, cache)); + checkResolution(resolveModuleName("linked", "/app/lib/main.ts", compilerOptions, host, cache)); + + function checkResolution(resolution: ResolvedModuleWithFailedLookupLocations) { + checkResolvedModule(resolution.resolvedModule, createResolvedModule("/linked/index.d.ts", /*isExternalLibraryImport*/ true)); + assert.strictEqual(resolution.resolvedModule!.originalPath, "/app/node_modules/linked/index.d.ts"); + } + }); }); describe("Module resolution - relative imports", () => { diff --git a/src/testRunner/unittests/tsserverProjectSystem.ts b/src/testRunner/unittests/tsserverProjectSystem.ts index c89d29ffc5b..355b2a4e7c6 100644 --- a/src/testRunner/unittests/tsserverProjectSystem.ts +++ b/src/testRunner/unittests/tsserverProjectSystem.ts @@ -8415,7 +8415,7 @@ new C();` expectedTrace.push(`Loading module '${moduleName}' from 'node_modules' folder, target file type 'TypeScript'.`); getExpectedMissedLocationResolutionTrace(host, expectedTrace, getDirectoryPath(file.path), module, moduleName, /*useNodeModules*/ true, cacheLocation); expectedTrace.push(`Resolution for module '${moduleName}' was found in cache from location '${cacheLocation}'.`); - getExpectedResolutionTraceFooter(expectedTrace, module, moduleName, /*addRealPathTrace*/ true, /*ignoreModuleFileFound*/ true); + getExpectedResolutionTraceFooter(expectedTrace, module, moduleName, /*addRealPathTrace*/ false, /*ignoreModuleFileFound*/ true); return expectedTrace; } diff --git a/tests/baselines/reference/cachedModuleResolution1.trace.json b/tests/baselines/reference/cachedModuleResolution1.trace.json index 48997f9784d..7f8ce2b403c 100644 --- a/tests/baselines/reference/cachedModuleResolution1.trace.json +++ b/tests/baselines/reference/cachedModuleResolution1.trace.json @@ -14,6 +14,5 @@ "Explicitly specified module resolution kind: 'NodeJs'.", "Loading module 'foo' from 'node_modules' folder, target file type 'TypeScript'.", "Resolution for module 'foo' was found in cache from location '/a/b/c'.", - "Resolving real path for '/a/b/node_modules/foo.d.ts', result '/a/b/node_modules/foo.d.ts'.", "======== Module name 'foo' was successfully resolved to '/a/b/node_modules/foo.d.ts'. ========" ] \ No newline at end of file diff --git a/tests/baselines/reference/cachedModuleResolution2.trace.json b/tests/baselines/reference/cachedModuleResolution2.trace.json index 197d3a708e3..3f9a7c532ea 100644 --- a/tests/baselines/reference/cachedModuleResolution2.trace.json +++ b/tests/baselines/reference/cachedModuleResolution2.trace.json @@ -14,6 +14,5 @@ "Directory '/a/b/c/d/e/node_modules' does not exist, skipping all lookups in it.", "Directory '/a/b/c/d/node_modules' does not exist, skipping all lookups in it.", "Resolution for module 'foo' was found in cache from location '/a/b/c'.", - "Resolving real path for '/a/b/node_modules/foo.d.ts', result '/a/b/node_modules/foo.d.ts'.", "======== Module name 'foo' was successfully resolved to '/a/b/node_modules/foo.d.ts'. ========" ] \ No newline at end of file diff --git a/tests/baselines/reference/cachedModuleResolution5.trace.json b/tests/baselines/reference/cachedModuleResolution5.trace.json index 4438192b155..adadb48efaf 100644 --- a/tests/baselines/reference/cachedModuleResolution5.trace.json +++ b/tests/baselines/reference/cachedModuleResolution5.trace.json @@ -14,6 +14,5 @@ "Explicitly specified module resolution kind: 'NodeJs'.", "Loading module 'foo' from 'node_modules' folder, target file type 'TypeScript'.", "Resolution for module 'foo' was found in cache from location '/a/b'.", - "Resolving real path for '/a/b/node_modules/foo.d.ts', result '/a/b/node_modules/foo.d.ts'.", "======== Module name 'foo' was successfully resolved to '/a/b/node_modules/foo.d.ts'. ========" ] \ No newline at end of file