From a416826b6453afa9bb447dd416a4013bb5db6019 Mon Sep 17 00:00:00 2001 From: Ryan Cavanaugh Date: Thu, 16 Nov 2017 12:37:40 -0800 Subject: [PATCH] Apply safelist exclusions even when include is not specified; recompute project contexts when installer reqs finish --- .../unittests/tsserverProjectSystem.ts | 4 +- src/harness/unittests/typingsInstaller.ts | 115 ++++++++++++------ src/harness/virtualFileSystemWithWatch.ts | 4 +- src/server/editorServices.ts | 34 ++++-- src/server/project.ts | 29 ++--- .../typingsInstaller/typingsInstaller.ts | 4 +- 6 files changed, 118 insertions(+), 72 deletions(-) diff --git a/src/harness/unittests/tsserverProjectSystem.ts b/src/harness/unittests/tsserverProjectSystem.ts index ea573726eb8..3525774b21c 100644 --- a/src/harness/unittests/tsserverProjectSystem.ts +++ b/src/harness/unittests/tsserverProjectSystem.ts @@ -1482,7 +1482,7 @@ namespace ts.projectSystem { it("ignores files excluded by a custom safe type list", () => { const file1 = { - path: "/a/b/f1.ts", + path: "/a/b/f1.js", content: "export let x = 5" }; const office = { @@ -1503,7 +1503,7 @@ namespace ts.projectSystem { it("ignores files excluded by the default type list", () => { const file1 = { - path: "/a/b/f1.ts", + path: "/a/b/f1.js", content: "export let x = 5" }; const minFile = { diff --git a/src/harness/unittests/typingsInstaller.ts b/src/harness/unittests/typingsInstaller.ts index e321fb1c99d..393d55a5ea4 100644 --- a/src/harness/unittests/typingsInstaller.ts +++ b/src/harness/unittests/typingsInstaller.ts @@ -304,35 +304,35 @@ namespace ts.projectSystem { // 1. react typings are installed for .jsx // 2. loose files names are matched against safe list for typings if // this is a JS project (only js, jsx, d.ts files are present) - const file1 = { + const lodashJs = { path: "/a/b/lodash.js", content: "" }; - const file2 = { + const file2Jsx = { path: "/a/b/file2.jsx", content: "" }; - const file3 = { + const file3dts = { path: "/a/b/file3.d.ts", content: "" }; - const react = { + const reactDts = { path: "/a/data/node_modules/@types/react/index.d.ts", content: "declare const react: { x: number }" }; - const lodash = { + const lodashDts = { path: "/a/data/node_modules/@types/lodash/index.d.ts", content: "declare const lodash: { x: number }" }; - const host = createServerHost([file1, file2, file3, customTypesMap]); + const host = createServerHost([lodashJs, file2Jsx, file3dts, customTypesMap]); const installer = new (class extends Installer { constructor() { super(host, { typesRegistry: createTypesRegistry("lodash", "react") }); } installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction): void { const installedTypings = ["@types/lodash", "@types/react"]; - const typingFiles = [lodash, react]; + const typingFiles = [lodashDts, reactDts]; executeCommand(this, host, installedTypings, typingFiles, cb); } })(); @@ -342,35 +342,31 @@ namespace ts.projectSystem { projectService.openExternalProject({ projectFileName, options: { allowJS: true, moduleResolution: ModuleResolutionKind.NodeJs }, - rootFiles: [toExternalFile(file1.path), toExternalFile(file2.path), toExternalFile(file3.path)], - typeAcquisition: {} + rootFiles: [toExternalFile(lodashJs.path), toExternalFile(file2Jsx.path), toExternalFile(file3dts.path)], + typeAcquisition: { } }); const p = projectService.externalProjects[0]; projectService.checkNumberOfProjects({ externalProjects: 1 }); - checkProjectActualFiles(p, [file1.path, file2.path, file3.path]); + checkProjectActualFiles(p, [file2Jsx.path, file3dts.path]); installer.installAll(/*expectedCount*/ 1); checkNumberOfProjects(projectService, { externalProjects: 1 }); host.checkTimeoutQueueLengthAndRun(2); checkNumberOfProjects(projectService, { externalProjects: 1 }); - checkProjectActualFiles(p, [file1.path, file2.path, file3.path, lodash.path, react.path]); + checkProjectActualFiles(p, [file2Jsx.path, file3dts.path, lodashDts.path, reactDts.path]); }); - it("external project - no type acquisition, with js & ts files", () => { + it("external project - type acquisition with enable: false", () => { // Tests: - // 1. No typings are included for JS projects when the project contains ts files - const file1 = { + // Exclude + const jqueryJs = { path: "/a/b/jquery.js", content: "" }; - const file2 = { - path: "/a/b/file2.ts", - content: "" - }; - const host = createServerHost([file1, file2]); + const host = createServerHost([jqueryJs]); const installer = new (class extends Installer { constructor() { super(host, { typesRegistry: createTypesRegistry("jquery") }); @@ -390,18 +386,62 @@ namespace ts.projectSystem { projectService.openExternalProject({ projectFileName, options: { allowJS: true, moduleResolution: ModuleResolutionKind.NodeJs }, - rootFiles: [toExternalFile(file1.path), toExternalFile(file2.path)], + rootFiles: [toExternalFile(jqueryJs.path)], + typeAcquisition: { enable: false } + }); + + const p = projectService.externalProjects[0]; + projectService.checkNumberOfProjects({ externalProjects: 1 }); + + checkProjectActualFiles(p, [jqueryJs.path]); + + installer.checkPendingCommands(/*expectedCount*/ 0); + }); + it("external project - no type acquisition, with js & ts files", () => { + // Tests: + // 1. No typings are included for JS projects when the project contains ts files + const jqueryJs = { + path: "/a/b/jquery.js", + content: "" + }; + const file2Ts = { + path: "/a/b/file2.ts", + content: "" + }; + + const host = createServerHost([jqueryJs, file2Ts]); + const installer = new (class extends Installer { + constructor() { + super(host, { typesRegistry: createTypesRegistry("jquery") }); + } + enqueueInstallTypingsRequest(project: server.Project, typeAcquisition: TypeAcquisition, unresolvedImports: server.SortedReadonlyArray) { + super.enqueueInstallTypingsRequest(project, typeAcquisition, unresolvedImports); + } + installWorker(_requestId: number, _args: string[], _cwd: string, cb: TI.RequestCompletedAction): void { + const installedTypings: string[] = []; + const typingFiles: FileOrFolder[] = []; + executeCommand(this, host, installedTypings, typingFiles, cb); + } + })(); + + const projectFileName = "/a/app/test.csproj"; + const projectService = createProjectService(host, { typingsInstaller: installer }); + projectService.openExternalProject({ + projectFileName, + options: { allowJS: true, moduleResolution: ModuleResolutionKind.NodeJs }, + rootFiles: [toExternalFile(jqueryJs.path), toExternalFile(file2Ts.path)], typeAcquisition: {} }); const p = projectService.externalProjects[0]; projectService.checkNumberOfProjects({ externalProjects: 1 }); - checkProjectActualFiles(p, [file2.path]); + + checkProjectActualFiles(p, [jqueryJs.path, file2Ts.path]); installer.checkPendingCommands(/*expectedCount*/ 0); checkNumberOfProjects(projectService, { externalProjects: 1 }); - checkProjectActualFiles(p, [file2.path]); + checkProjectActualFiles(p, [jqueryJs.path, file2Ts.path]); }); it("external project - with type acquisition, with only js, d.ts files", () => { @@ -409,15 +449,15 @@ namespace ts.projectSystem { // 1. Safelist matching, type acquisition includes/excludes and package.json typings are all acquired // 2. Types for safelist matches are not included when they also appear in the type acquisition exclude list // 3. Multiple includes and excludes are respected in type acquisition - const file1 = { + const lodashJs = { path: "/a/b/lodash.js", content: "" }; - const file2 = { + const commanderJs = { path: "/a/b/commander.js", content: "" }; - const file3 = { + const file3dts = { path: "/a/b/file3.d.ts", content: "" }; @@ -448,7 +488,7 @@ namespace ts.projectSystem { content: "declare const moment: { x: number }" }; - const host = createServerHost([file1, file2, file3, packageJson, customTypesMap]); + const host = createServerHost([lodashJs, commanderJs, file3dts, packageJson, customTypesMap]); const installer = new (class extends Installer { constructor() { super(host, { typesRegistry: createTypesRegistry("jquery", "commander", "moment", "express") }); @@ -465,20 +505,25 @@ namespace ts.projectSystem { projectService.openExternalProject({ projectFileName, options: { allowJS: true, moduleResolution: ModuleResolutionKind.NodeJs }, - rootFiles: [toExternalFile(file1.path), toExternalFile(file2.path), toExternalFile(file3.path)], - typeAcquisition: { include: ["jquery", "moment"], exclude: ["lodash"] } + rootFiles: [toExternalFile(lodashJs.path), toExternalFile(commanderJs.path), toExternalFile(file3dts.path)], + typeAcquisition: { enable: true, include: ["jquery", "moment"], exclude: ["lodash"] } }); const p = projectService.externalProjects[0]; projectService.checkNumberOfProjects({ externalProjects: 1 }); - checkProjectActualFiles(p, [file1.path, file2.path, file3.path]); + checkProjectActualFiles(p, [file3dts.path]); installer.installAll(/*expectedCount*/ 1); checkNumberOfProjects(projectService, { externalProjects: 1 }); host.checkTimeoutQueueLengthAndRun(2); checkNumberOfProjects(projectService, { externalProjects: 1 }); - checkProjectActualFiles(p, [file1.path, file2.path, file3.path, commander.path, express.path, jquery.path, moment.path]); + // Commander: Existed as a JS file + // JQuery: Specified in 'include' + // Moment: Specified in 'include' + // Express: Specified in package.json + // lodash: Excluded (not present) + checkProjectActualFiles(p, [file3dts.path, commander.path, jquery.path, moment.path, express.path]); }); it("Throttle - delayed typings to install", () => { @@ -548,7 +593,7 @@ namespace ts.projectSystem { const p = projectService.externalProjects[0]; projectService.checkNumberOfProjects({ externalProjects: 1 }); - checkProjectActualFiles(p, [lodashJs.path, commanderJs.path, file3.path]); + checkProjectActualFiles(p, [file3.path]); installer.checkPendingCommands(/*expectedCount*/ 1); installer.executePendingCommands(); // expected all typings file to exist @@ -557,7 +602,7 @@ namespace ts.projectSystem { } host.checkTimeoutQueueLengthAndRun(2); checkNumberOfProjects(projectService, { externalProjects: 1 }); - checkProjectActualFiles(p, [lodashJs.path, commanderJs.path, file3.path, commander.path, express.path, jquery.path, moment.path, lodash.path]); + checkProjectActualFiles(p, [file3.path, commander.path, express.path, jquery.path, moment.path, lodash.path]); }); it("Throttle - delayed run install requests", () => { @@ -648,7 +693,7 @@ namespace ts.projectSystem { const p1 = projectService.externalProjects[0]; const p2 = projectService.externalProjects[1]; projectService.checkNumberOfProjects({ externalProjects: 2 }); - checkProjectActualFiles(p1, [lodashJs.path, commanderJs.path, file3.path]); + checkProjectActualFiles(p1, [file3.path]); checkProjectActualFiles(p2, [file3.path]); installer.executePendingCommands(); @@ -659,7 +704,7 @@ namespace ts.projectSystem { installer.executePendingCommands(); host.checkTimeoutQueueLengthAndRun(3); // for 2 projects and 1 refreshing inferred project - checkProjectActualFiles(p1, [lodashJs.path, commanderJs.path, file3.path, commander.path, jquery.path, lodash.path, cordova.path]); + checkProjectActualFiles(p1, [file3.path, commander.path, jquery.path, lodash.path, cordova.path]); checkProjectActualFiles(p2, [file3.path, grunt.path, gulp.path]); }); @@ -973,7 +1018,7 @@ namespace ts.projectSystem { } }; session.executeCommand(changeRequest); - host.checkTimeoutQueueLengthAndRun(2); // This enqueues the updategraph and refresh inferred projects + host.checkTimeoutQueueLengthAndRun(0); // This enqueues the updategraph and refresh inferred projects const version2 = proj.getCachedUnresolvedImportsPerFile_TestOnly().getVersion(); assert.equal(version1, version2, "set of unresolved imports should not change"); }); diff --git a/src/harness/virtualFileSystemWithWatch.ts b/src/harness/virtualFileSystemWithWatch.ts index 25581093f3d..d4d203cabbb 100644 --- a/src/harness/virtualFileSystemWithWatch.ts +++ b/src/harness/virtualFileSystemWithWatch.ts @@ -144,9 +144,9 @@ interface Array {}` } export function checkFileNames(caption: string, actualFileNames: ReadonlyArray, expectedFileNames: string[]) { - assert.equal(actualFileNames.length, expectedFileNames.length, `${caption}: incorrect actual number of files, expected ${expectedFileNames}, got ${actualFileNames}`); + assert.equal(actualFileNames.length, expectedFileNames.length, `${caption}: incorrect actual number of files, expected:\r\n${expectedFileNames.join("\r\n")}\r\ngot: ${actualFileNames.join("\r\n")}`); for (const f of expectedFileNames) { - assert.isTrue(contains(actualFileNames, f), `${caption}: expected to find ${f} in ${actualFileNames}`); + assert.equal(true, contains(actualFileNames, f), `${caption}: expected to find ${f} in ${actualFileNames}`); } } diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 726621a540d..00fbdf52809 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -2193,8 +2193,13 @@ namespace ts.server { applySafeList(proj: protocol.ExternalProject): NormalizedPath[] { const { rootFiles, typeAcquisition } = proj; - const types = (typeAcquisition && typeAcquisition.include) || []; + Debug.assert(!!typeAcquisition, "proj.typeAcquisition should be set by now"); + // If type acquisition has been explicitly disabled, do not exclude anything from the project + if (typeAcquisition.enable === false) { + return []; + } + const typeAcqInclude = typeAcquisition.include || (typeAcquisition.include = []); const excludeRules: string[] = []; const normalizedNames = rootFiles.map(f => normalizeSlashes(f.fileName)) as NormalizedPath[]; @@ -2209,8 +2214,10 @@ namespace ts.server { // If the file matches, collect its types packages and exclude rules if (rule.types) { for (const type of rule.types) { - if (types.indexOf(type) < 0) { - types.push(type); + // Best-effort de-duping here - doesn't need to be unduplicated but + // we don't want the list to become a 400-element array of just 'kendo' + if (typeAcqInclude.indexOf(type) < 0) { + typeAcqInclude.push(type); } } } @@ -2248,12 +2255,6 @@ namespace ts.server { } } } - - // Copy back this field into the project if needed - if (types.length > 0) { - proj.typeAcquisition = proj.typeAcquisition || {}; - proj.typeAcquisition.include = types; - } } const excludeRegexes = excludeRules.map(e => new RegExp(e, "i")); @@ -2264,7 +2265,7 @@ namespace ts.server { } else { let exclude = false; - if (typeAcquisition && (typeAcquisition.enable || typeAcquisition.enableAutoDiscovery)) { + if (typeAcquisition.enable || typeAcquisition.enableAutoDiscovery) { const baseName = getBaseFileName(normalizedNames[i].toLowerCase()); if (fileExtensionIs(baseName, "js")) { const inferredTypingName = removeFileExtension(baseName); @@ -2272,7 +2273,14 @@ namespace ts.server { if (this.legacySafelist[cleanedTypingName]) { this.logger.info(`Excluded '${normalizedNames[i]}' because it matched ${cleanedTypingName} from the legacy safelist`); excludedFiles.push(normalizedNames[i]); + // *exclude* it from the project... exclude = true; + // ... but *include* it in the list of types to acquire + const typeName = this.legacySafelist[cleanedTypingName]; + // Same best-effort dedupe as above + if (typeAcqInclude.indexOf(typeName) < 0) { + typeAcqInclude.push(typeName); + } } } } @@ -2292,6 +2300,12 @@ namespace ts.server { const typeAcquisition = convertEnableAutoDiscoveryToEnable(proj.typingOptions); proj.typeAcquisition = typeAcquisition; } + proj.typeAcquisition = proj.typeAcquisition || {}; + proj.typeAcquisition.include = proj.typeAcquisition.include || []; + proj.typeAcquisition.exclude = proj.typeAcquisition.exclude || []; + if (proj.typeAcquisition.enable === undefined) { + proj.typeAcquisition.enable = hasNoTypeScriptSource(proj.rootFiles.map(f => f.fileName)); + } const excludedFiles = this.applySafeList(proj); diff --git a/src/server/project.ts b/src/server/project.ts index d2526ab2abb..8988e8b1bec 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -52,6 +52,11 @@ namespace ts.server { return counts.ts === 0 && counts.tsx === 0; } + /* @internal */ + export function hasNoTypeScriptSource(fileNames: string[]): boolean { + return !fileNames.some(fileName => (fileExtensionIs(fileName, Extension.Ts) && !fileExtensionIs(fileName, Extension.Dts)) || fileExtensionIs(fileName, Extension.Tsx)); + } + /* @internal */ export interface ProjectFilesWithTSDiagnostics extends protocol.ProjectFiles { projectErrors: ReadonlyArray; @@ -1436,26 +1441,10 @@ namespace ts.server { } setTypeAcquisition(newTypeAcquisition: TypeAcquisition): void { - if (!newTypeAcquisition) { - // set default typings options - newTypeAcquisition = { - enable: allRootFilesAreJsOrDts(this), - include: [], - exclude: [] - }; - } - else { - if (newTypeAcquisition.enable === undefined) { - // if autoDiscovery was not specified by the caller - set it based on the content of the project - newTypeAcquisition.enable = allRootFilesAreJsOrDts(this); - } - if (!newTypeAcquisition.include) { - newTypeAcquisition.include = []; - } - if (!newTypeAcquisition.exclude) { - newTypeAcquisition.exclude = []; - } - } + Debug.assert(!!newTypeAcquisition, "newTypeAcquisition may not be null/undefined"); + Debug.assert(!!newTypeAcquisition.include, "newTypeAcquisition.include may not be null/undefined"); + Debug.assert(!!newTypeAcquisition.exclude, "newTypeAcquisition.exclude may not be null/undefined"); + Debug.assert(typeof newTypeAcquisition.enable === "boolean", "newTypeAcquisition.enable may not be null/undefined"); this.typeAcquisition = newTypeAcquisition; } } diff --git a/src/server/typingsInstaller/typingsInstaller.ts b/src/server/typingsInstaller/typingsInstaller.ts index eacbebbf4ab..63bbc219789 100644 --- a/src/server/typingsInstaller/typingsInstaller.ts +++ b/src/server/typingsInstaller/typingsInstaller.ts @@ -123,9 +123,6 @@ namespace ts.server.typingsInstaller { this.log.writeLine(`Finished typings discovery: ${JSON.stringify(discoverTypingsResult)}`); } - // respond with whatever cached typings we have now - this.sendResponse(this.createSetTypings(req, discoverTypingsResult.cachedTypingPaths)); - // start watching files this.watchFiles(req.projectName, discoverTypingsResult.filesToWatch); @@ -134,6 +131,7 @@ namespace ts.server.typingsInstaller { this.installTypings(req, req.cachePath || this.globalCachePath, discoverTypingsResult.cachedTypingPaths, discoverTypingsResult.newTypingNames); } else { + this.sendResponse(this.createSetTypings(req, discoverTypingsResult.cachedTypingPaths)); if (this.log.isEnabled()) { this.log.writeLine(`No new typings were requested as a result of typings discovery`); }