From abbf8f15d55614c9fa1d420f9de1b637eb8bb125 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 25 Sep 2018 09:00:22 -0700 Subject: [PATCH 1/4] Fix bug: ATA should still install types if a package was resolved to a '.js' file --- src/server/project.ts | 73 ++++++++------------ src/testRunner/unittests/typingsInstaller.ts | 38 +++++----- 2 files changed, 48 insertions(+), 63 deletions(-) diff --git a/src/server/project.ts b/src/server/project.ts index e0297c640b82a..b90be2f2aa30d 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -792,41 +792,6 @@ namespace ts.server { } } - /* @internal */ - private extractUnresolvedImportsFromSourceFile(file: SourceFile, ambientModules: string[]): ReadonlyArray { - const cached = this.cachedUnresolvedImportsPerFile.get(file.path); - if (cached) { - // found cached result, return - return cached; - } - let unresolvedImports: string[] | undefined; - if (file.resolvedModules) { - file.resolvedModules.forEach((resolvedModule, name) => { - // pick unresolved non-relative names - if (!resolvedModule && !isExternalModuleNameRelative(name) && !isAmbientlyDeclaredModule(name)) { - // for non-scoped names extract part up-to the first slash - // for scoped names - extract up to the second slash - let trimmed = name.trim(); - let i = trimmed.indexOf("/"); - if (i !== -1 && trimmed.charCodeAt(0) === CharacterCodes.at) { - i = trimmed.indexOf("/", i + 1); - } - if (i !== -1) { - trimmed = trimmed.substr(0, i); - } - (unresolvedImports || (unresolvedImports = [])).push(trimmed); - } - }); - } - - this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray); - return unresolvedImports || emptyArray; - - function isAmbientlyDeclaredModule(name: string) { - return ambientModules.some(m => m === name); - } - } - /* @internal */ onFileAddedOrRemoved() { this.hasAddedorRemovedFiles = true; @@ -860,15 +825,7 @@ namespace ts.server { // (can reuse cached imports for files that were not changed) // 4. compilation settings were changed in the way that might affect module resolution - drop all caches and collect all data from the scratch if (hasNewProgram || changedFiles.length) { - let result: string[] | undefined; - const ambientModules = this.program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName())); - for (const sourceFile of this.program.getSourceFiles()) { - const unResolved = this.extractUnresolvedImportsFromSourceFile(sourceFile, ambientModules); - if (unResolved !== emptyArray) { - (result || (result = [])).push(...unResolved); - } - } - this.lastCachedUnresolvedImportsList = result ? toDeduplicatedSortedArray(result) : emptyArray; + this.lastCachedUnresolvedImportsList = getUnresolvedImports(this.program, this.cachedUnresolvedImportsPerFile); } this.projectService.typingsCache.enqueueInstallTypingsForProject(this, this.lastCachedUnresolvedImportsList, hasAddedorRemovedFiles); @@ -1188,6 +1145,34 @@ namespace ts.server { } } + function getUnresolvedImports(program: Program, cachedUnresolvedImportsPerFile: Map>): SortedReadonlyArray { + const ambientModules = program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName())); + return toDeduplicatedSortedArray(flatMap(program.getSourceFiles(), sourceFile => + extractUnresolvedImportsFromSourceFile(sourceFile, ambientModules, cachedUnresolvedImportsPerFile))); + } + + function extractUnresolvedImportsFromSourceFile(file: SourceFile, ambientModules: ReadonlyArray, cachedUnresolvedImportsPerFile: Map>): ReadonlyArray { + const cached = cachedUnresolvedImportsPerFile.get(file.path); + if (cached) { + // found cached result, return + return cached; + } + let unresolvedImports: string[] | undefined; + if (file.resolvedModules) { + file.resolvedModules.forEach((resolvedModule, name) => { + // pick unresolved non-relative names + if ((!resolvedModule || !extensionIsTS(resolvedModule.extension)) && + !isExternalModuleNameRelative(name) && + !ambientModules.some(m => m === name)) { + unresolvedImports = append(unresolvedImports, parsePackageName(name).packageName); + } + }); + } + + cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray); + return unresolvedImports || emptyArray; + } + /** * If a file is opened and no tsconfig (or jsconfig) is found, * the file and its imports/references are put into an InferredProject. diff --git a/src/testRunner/unittests/typingsInstaller.ts b/src/testRunner/unittests/typingsInstaller.ts index a718a12f5484f..d862ecbfeed04 100644 --- a/src/testRunner/unittests/typingsInstaller.ts +++ b/src/testRunner/unittests/typingsInstaller.ts @@ -945,29 +945,28 @@ namespace ts.projectSystem { }); it("should install typings for unresolved imports", () => { - const file = { + const file: TestFSWithWatch.File = { path: "/a/b/app.js", content: ` import * as fs from "fs"; import * as commander from "commander";` }; - const cachePath = "/a/cache"; - const node = { - path: cachePath + "/node_modules/@types/node/index.d.ts", - content: "export let x: number" + const commanderJS: TestFSWithWatch.File = { + path: "/node_modules/commander/index.js", + content: "module.exports = 0", }; - const commander = { - path: cachePath + "/node_modules/@types/commander/index.d.ts", - content: "export let y: string" - }; - const host = createServerHost([file]); + + const cachePath = "/a/cache"; + const typeNames: ReadonlyArray = ["node", "commander"]; + const typePath = (name: string): string => `${cachePath}/node_modules/@types/${name}/index.d.ts`; + const host = createServerHost([file, commanderJS]); const installer = new (class extends Installer { constructor() { - super(host, { globalTypingsCacheLocation: cachePath, typesRegistry: createTypesRegistry("node", "commander") }); + super(host, { globalTypingsCacheLocation: cachePath, typesRegistry: createTypesRegistry(...typeNames) }); } installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction) { - const installedTypings = ["@types/node", "@types/commander"]; - const typingFiles = [node, commander]; + const installedTypings = typeNames.map(name => `@types/${name}`); + const typingFiles = typeNames.map((name): TestFSWithWatch.File => ({ path: typePath(name), content: "" })); executeCommand(this, host, installedTypings, typingFiles, cb); } })(); @@ -975,15 +974,16 @@ namespace ts.projectSystem { service.openClientFile(file.path); service.checkNumberOfProjects({ inferredProjects: 1 }); - checkProjectActualFiles(service.inferredProjects[0], [file.path]); + checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path]); installer.installAll(/*expectedCount*/1); - - assert.isTrue(host.fileExists(node.path), "typings for 'node' should be created"); - assert.isTrue(host.fileExists(commander.path), "typings for 'commander' should be created"); - + for (const name of typeNames) { + assert.isTrue(host.fileExists(typePath(name)), `typings for '${name}' should be created`); + } host.checkTimeoutQueueLengthAndRun(2); - checkProjectActualFiles(service.inferredProjects[0], [file.path, node.path, commander.path]); + + // TODO: GH#27302 Should update the project to not include commanderJS.path when @types/commander is installed. Currently it sticks with the JS file. + checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path, ...typeNames.map(typePath)]); }); it("should pick typing names from non-relative unresolved imports", () => { From 4dd93f92c6bef05a8fc2859e02936b21969ea7bd Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 26 Sep 2018 13:52:14 -0700 Subject: [PATCH 2/4] Ignore .json modules --- src/server/project.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/project.ts b/src/server/project.ts index b90be2f2aa30d..69ef1d125bc50 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -1161,7 +1161,7 @@ namespace ts.server { if (file.resolvedModules) { file.resolvedModules.forEach((resolvedModule, name) => { // pick unresolved non-relative names - if ((!resolvedModule || !extensionIsTS(resolvedModule.extension)) && + if ((!resolvedModule || !resolutionExtensionIsTSOrJson(resolvedModule.extension)) && !isExternalModuleNameRelative(name) && !ambientModules.some(m => m === name)) { unresolvedImports = append(unresolvedImports, parsePackageName(name).packageName); From 67bbf1513919d056d88c11119bf70478a7671956 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Wed, 26 Sep 2018 14:23:43 -0700 Subject: [PATCH 3/4] Fix bugs relating to watching the cache --- src/compiler/resolutionCache.ts | 28 +++++--- src/compiler/utilities.ts | 4 +- src/server/project.ts | 73 ++++++++++++-------- src/testRunner/unittests/typingsInstaller.ts | 16 +++-- 4 files changed, 77 insertions(+), 44 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index 33e6dcd1221c1..af62f8c6f1e87 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -217,26 +217,29 @@ namespace ts { }); } +<<<<<<< HEAD function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost): CachedResolvedModuleWithFailedLookupLocations { const primaryResult = ts.resolveModuleName(moduleName, containingFile, compilerOptions, host, moduleResolutionCache); // return result immediately only if global cache support is not enabled or if it is .ts, .tsx or .d.ts if (!resolutionHost.getGlobalCache) { return primaryResult; } +======= + function resolveModuleName(moduleName: string, containingFile: string, compilerOptions: CompilerOptions, host: ModuleResolutionHost, redirectedReference?: ResolvedProjectReference): CachedResolvedModuleWithFailedLookupLocations { + const primaryResult = ts.resolveModuleName(moduleName, containingFile, compilerOptions, host, moduleResolutionCache, redirectedReference); +>>>>>>> 9db864e... Fix bugs relating to watching the cache - // otherwise try to load typings from @types - const globalCache = resolutionHost.getGlobalCache(); + const globalCache = resolutionHost.getGlobalCache ? resolutionHost.getGlobalCache() : undefined; if (globalCache !== undefined && !isExternalModuleNameRelative(moduleName) && !(primaryResult.resolvedModule && extensionIsTS(primaryResult.resolvedModule.extension))) { // create different collection of failed lookup locations for second pass // if it will fail and we've already found something during the first pass - we don't want to pollute its results const { resolvedModule, failedLookupLocations } = loadModuleFromGlobalCache(moduleName, resolutionHost.projectName, compilerOptions, host, globalCache); - if (resolvedModule) { - return { resolvedModule, failedLookupLocations: addRange(primaryResult.failedLookupLocations as string[], failedLookupLocations) }; - } + return { resolvedModule: resolvedModule || primaryResult.resolvedModule, failedLookupLocations: [...primaryResult.failedLookupLocations, ...failedLookupLocations] }; + } + else { + // Default return the result from the first pass + return primaryResult; } - - // Default return the result from the first pass - return primaryResult; } function resolveNamesWithLocalCache( @@ -379,10 +382,17 @@ namespace ts { return true; } + const globalCache = resolutionHost.getGlobalCache ? resolutionHost.getGlobalCache() : undefined; + // Using identity for getCanonicalFileName because assuming that if a path came from globalCache its case hasn't been changed. + if (globalCache !== undefined && startsWithDirectory(dirPath, globalCache, identity)) { + return true; + } + + // Path must have at least 3 components for (let searchIndex = nextDirectorySeparator + 1, searchLevels = 2; searchLevels > 0; searchLevels--) { searchIndex = dirPath.indexOf(directorySeparator, searchIndex) + 1; if (searchIndex === 0) { - // Folder isnt at expected minimun levels + // Folder isnt at expected minimum levels return false; } } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 60a7d759b688a..8e512d1ed776c 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -7293,7 +7293,9 @@ namespace ts { export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean { const canonicalFileName = getCanonicalFileName(fileName); const canonicalDirectoryName = getCanonicalFileName(directoryName); - return startsWith(canonicalFileName, canonicalDirectoryName + "/") || startsWith(canonicalFileName, canonicalDirectoryName + "\\"); + return canonicalFileName === canonicalDirectoryName || + startsWith(canonicalFileName, canonicalDirectoryName + "/") || + startsWith(canonicalFileName, canonicalDirectoryName + "\\"); } export function isUrl(path: string) { diff --git a/src/server/project.ts b/src/server/project.ts index 69ef1d125bc50..e0297c640b82a 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -792,6 +792,41 @@ namespace ts.server { } } + /* @internal */ + private extractUnresolvedImportsFromSourceFile(file: SourceFile, ambientModules: string[]): ReadonlyArray { + const cached = this.cachedUnresolvedImportsPerFile.get(file.path); + if (cached) { + // found cached result, return + return cached; + } + let unresolvedImports: string[] | undefined; + if (file.resolvedModules) { + file.resolvedModules.forEach((resolvedModule, name) => { + // pick unresolved non-relative names + if (!resolvedModule && !isExternalModuleNameRelative(name) && !isAmbientlyDeclaredModule(name)) { + // for non-scoped names extract part up-to the first slash + // for scoped names - extract up to the second slash + let trimmed = name.trim(); + let i = trimmed.indexOf("/"); + if (i !== -1 && trimmed.charCodeAt(0) === CharacterCodes.at) { + i = trimmed.indexOf("/", i + 1); + } + if (i !== -1) { + trimmed = trimmed.substr(0, i); + } + (unresolvedImports || (unresolvedImports = [])).push(trimmed); + } + }); + } + + this.cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray); + return unresolvedImports || emptyArray; + + function isAmbientlyDeclaredModule(name: string) { + return ambientModules.some(m => m === name); + } + } + /* @internal */ onFileAddedOrRemoved() { this.hasAddedorRemovedFiles = true; @@ -825,7 +860,15 @@ namespace ts.server { // (can reuse cached imports for files that were not changed) // 4. compilation settings were changed in the way that might affect module resolution - drop all caches and collect all data from the scratch if (hasNewProgram || changedFiles.length) { - this.lastCachedUnresolvedImportsList = getUnresolvedImports(this.program, this.cachedUnresolvedImportsPerFile); + let result: string[] | undefined; + const ambientModules = this.program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName())); + for (const sourceFile of this.program.getSourceFiles()) { + const unResolved = this.extractUnresolvedImportsFromSourceFile(sourceFile, ambientModules); + if (unResolved !== emptyArray) { + (result || (result = [])).push(...unResolved); + } + } + this.lastCachedUnresolvedImportsList = result ? toDeduplicatedSortedArray(result) : emptyArray; } this.projectService.typingsCache.enqueueInstallTypingsForProject(this, this.lastCachedUnresolvedImportsList, hasAddedorRemovedFiles); @@ -1145,34 +1188,6 @@ namespace ts.server { } } - function getUnresolvedImports(program: Program, cachedUnresolvedImportsPerFile: Map>): SortedReadonlyArray { - const ambientModules = program.getTypeChecker().getAmbientModules().map(mod => stripQuotes(mod.getName())); - return toDeduplicatedSortedArray(flatMap(program.getSourceFiles(), sourceFile => - extractUnresolvedImportsFromSourceFile(sourceFile, ambientModules, cachedUnresolvedImportsPerFile))); - } - - function extractUnresolvedImportsFromSourceFile(file: SourceFile, ambientModules: ReadonlyArray, cachedUnresolvedImportsPerFile: Map>): ReadonlyArray { - const cached = cachedUnresolvedImportsPerFile.get(file.path); - if (cached) { - // found cached result, return - return cached; - } - let unresolvedImports: string[] | undefined; - if (file.resolvedModules) { - file.resolvedModules.forEach((resolvedModule, name) => { - // pick unresolved non-relative names - if ((!resolvedModule || !resolutionExtensionIsTSOrJson(resolvedModule.extension)) && - !isExternalModuleNameRelative(name) && - !ambientModules.some(m => m === name)) { - unresolvedImports = append(unresolvedImports, parsePackageName(name).packageName); - } - }); - } - - cachedUnresolvedImportsPerFile.set(file.path, unresolvedImports || emptyArray); - return unresolvedImports || emptyArray; - } - /** * If a file is opened and no tsconfig (or jsconfig) is found, * the file and its imports/references are put into an InferredProject. diff --git a/src/testRunner/unittests/typingsInstaller.ts b/src/testRunner/unittests/typingsInstaller.ts index d862ecbfeed04..ca3eadc9bae21 100644 --- a/src/testRunner/unittests/typingsInstaller.ts +++ b/src/testRunner/unittests/typingsInstaller.ts @@ -951,15 +951,19 @@ namespace ts.projectSystem { import * as fs from "fs"; import * as commander from "commander";` }; + const cachePath = "/a/cache"; + const node = { + path: cachePath + "/node_modules/@types/node/index.d.ts", + content: "export let x: number", + }; const commanderJS: TestFSWithWatch.File = { path: "/node_modules/commander/index.js", content: "module.exports = 0", }; - const cachePath = "/a/cache"; const typeNames: ReadonlyArray = ["node", "commander"]; const typePath = (name: string): string => `${cachePath}/node_modules/@types/${name}/index.d.ts`; - const host = createServerHost([file, commanderJS]); + const host = createServerHost([file, node, commanderJS]); const installer = new (class extends Installer { constructor() { super(host, { globalTypingsCacheLocation: cachePath, typesRegistry: createTypesRegistry(...typeNames) }); @@ -973,6 +977,10 @@ namespace ts.projectSystem { const service = createProjectService(host, { typingsInstaller: installer }); service.openClientFile(file.path); + checkWatchedFiles(host, [...flatMap(["/a/b", "/a", ""], x => [x + "/tsconfig.json", x + "/jsconfig.json"]), "/a/lib/lib.d.ts"]); + checkWatchedDirectories(host, [], /*recursive*/ false); + checkWatchedDirectories(host, ["/node_modules", "/a/b/node_modules", "/a/cache/node_modules", "/a/b/node_modules/@types", "/a/b/bower_components"], /*recursive*/ true); + service.checkNumberOfProjects({ inferredProjects: 1 }); checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path]); @@ -981,9 +989,7 @@ namespace ts.projectSystem { assert.isTrue(host.fileExists(typePath(name)), `typings for '${name}' should be created`); } host.checkTimeoutQueueLengthAndRun(2); - - // TODO: GH#27302 Should update the project to not include commanderJS.path when @types/commander is installed. Currently it sticks with the JS file. - checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path, ...typeNames.map(typePath)]); + checkProjectActualFiles(service.inferredProjects[0], [file.path, ...typeNames.map(typePath)]); }); it("should pick typing names from non-relative unresolved imports", () => { From 76e730ee5a53a9fa2d98504b4051b4fd3e4b444c Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 30 Oct 2018 10:48:11 -0700 Subject: [PATCH 4/4] Remove old comment, and check for globalCache exactly instead of using startsWithDirectory --- src/compiler/resolutionCache.ts | 3 +-- src/compiler/utilities.ts | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index 8e9c7457be1d2..e242e7e0b1963 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -224,7 +224,6 @@ namespace ts { const globalCache = resolutionHost.getGlobalCache ? resolutionHost.getGlobalCache() : undefined; if (globalCache !== undefined && !isExternalModuleNameRelative(moduleName) && !(primaryResult.resolvedModule && extensionIsTS(primaryResult.resolvedModule.extension))) { // create different collection of failed lookup locations for second pass - // if it will fail and we've already found something during the first pass - we don't want to pollute its results const { resolvedModule, failedLookupLocations } = loadModuleFromGlobalCache(moduleName, resolutionHost.projectName, compilerOptions, host, globalCache); return { resolvedModule: resolvedModule || primaryResult.resolvedModule, failedLookupLocations: [...primaryResult.failedLookupLocations, ...failedLookupLocations] }; } @@ -386,7 +385,7 @@ namespace ts { const globalCache = resolutionHost.getGlobalCache ? resolutionHost.getGlobalCache() : undefined; // Using identity for getCanonicalFileName because assuming that if a path came from globalCache its case hasn't been changed. - if (globalCache !== undefined && startsWithDirectory(dirPath, globalCache, identity)) { + if (globalCache !== undefined && dirPath === globalCache) { return true; } diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index a620ea64d7e81..a969d580b86a9 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -7333,9 +7333,7 @@ namespace ts { export function startsWithDirectory(fileName: string, directoryName: string, getCanonicalFileName: GetCanonicalFileName): boolean { const canonicalFileName = getCanonicalFileName(fileName); const canonicalDirectoryName = getCanonicalFileName(directoryName); - return canonicalFileName === canonicalDirectoryName || - startsWith(canonicalFileName, canonicalDirectoryName + "/") || - startsWith(canonicalFileName, canonicalDirectoryName + "\\"); + return startsWith(canonicalFileName, canonicalDirectoryName + "/") || startsWith(canonicalFileName, canonicalDirectoryName + "\\"); } export function isUrl(path: string) {