From c81138b0acae2c6d8b99311f78bb88f1a58d1ab4 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Fri, 3 Jan 2020 16:25:53 -0800 Subject: [PATCH 1/2] Add test that demonstrates npm install watch behaviour some times --- src/harness/virtualFileSystemWithWatch.ts | 15 +-- .../unittests/tsserver/projectErrors.ts | 114 ++++++++++++++++++ 2 files changed, 122 insertions(+), 7 deletions(-) diff --git a/src/harness/virtualFileSystemWithWatch.ts b/src/harness/virtualFileSystemWithWatch.ts index 3236e85c37607..561b51e7556c5 100644 --- a/src/harness/virtualFileSystemWithWatch.ts +++ b/src/harness/virtualFileSystemWithWatch.ts @@ -611,28 +611,29 @@ interface Array { length: number; [n: number]: T; }` } } - ensureFileOrFolder(fileOrDirectoryOrSymLink: FileOrFolderOrSymLink, ignoreWatchInvokedWithTriggerAsFileCreate?: boolean) { + ensureFileOrFolder(fileOrDirectoryOrSymLink: FileOrFolderOrSymLink, ignoreWatchInvokedWithTriggerAsFileCreate?: boolean, ignoreParentWatch?: boolean) { if (isFile(fileOrDirectoryOrSymLink)) { const file = this.toFsFile(fileOrDirectoryOrSymLink); // file may already exist when updating existing type declaration file if (!this.fs.get(file.path)) { - const baseFolder = this.ensureFolder(getDirectoryPath(file.fullPath)); + const baseFolder = this.ensureFolder(getDirectoryPath(file.fullPath), ignoreParentWatch); this.addFileOrFolderInFolder(baseFolder, file, ignoreWatchInvokedWithTriggerAsFileCreate); } } else if (isSymLink(fileOrDirectoryOrSymLink)) { const symLink = this.toFsSymLink(fileOrDirectoryOrSymLink); Debug.assert(!this.fs.get(symLink.path)); - const baseFolder = this.ensureFolder(getDirectoryPath(symLink.fullPath)); + const baseFolder = this.ensureFolder(getDirectoryPath(symLink.fullPath), ignoreParentWatch); this.addFileOrFolderInFolder(baseFolder, symLink, ignoreWatchInvokedWithTriggerAsFileCreate); } else { const fullPath = getNormalizedAbsolutePath(fileOrDirectoryOrSymLink.path, this.currentDirectory); - this.ensureFolder(fullPath); + this.ensureFolder(getDirectoryPath(fullPath), ignoreParentWatch); + this.ensureFolder(fullPath, ignoreWatchInvokedWithTriggerAsFileCreate); } } - private ensureFolder(fullPath: string): FsFolder { + private ensureFolder(fullPath: string, ignoreWatch: boolean | undefined): FsFolder { const path = this.toPath(fullPath); let folder = this.fs.get(path) as FsFolder; if (!folder) { @@ -640,8 +641,8 @@ interface Array { length: number; [n: number]: T; }` const baseFullPath = getDirectoryPath(fullPath); if (fullPath !== baseFullPath) { // Add folder in the base folder - const baseFolder = this.ensureFolder(baseFullPath); - this.addFileOrFolderInFolder(baseFolder, folder); + const baseFolder = this.ensureFolder(baseFullPath, ignoreWatch); + this.addFileOrFolderInFolder(baseFolder, folder, ignoreWatch); } else { // root folder diff --git a/src/testRunner/unittests/tsserver/projectErrors.ts b/src/testRunner/unittests/tsserver/projectErrors.ts index e857cb8108f66..1c71628acff36 100644 --- a/src/testRunner/unittests/tsserver/projectErrors.ts +++ b/src/testRunner/unittests/tsserver/projectErrors.ts @@ -933,4 +933,118 @@ console.log(blabla);` }); }); }); + + describe("unittests:: tsserver:: Project Errors with npm install when", () => { + function verifyNpmInstall(timeoutDuringPartialInstallation: boolean) { + const main: File = { + path: `${tscWatch.projectRoot}/src/main.ts`, + content: "import * as _a from '@angular/core';" + }; + const config: File = { + path: `${tscWatch.projectRoot}/tsconfig.json`, + content: "{}" + }; + const projectFiles = [main, libFile, config]; + const host = createServerHost(projectFiles); + const session = createSession(host, { canUseEvents: true }); + const service = session.getProjectService(); + openFilesForSession([{ file: main, projectRootPath: tscWatch.projectRoot }], session); + const span = protocolTextSpanFromSubstring(main.content, `'@angular/core'`); + const moduleNotFoundErr: protocol.Diagnostic[] = [ + createDiagnostic( + span.start, + span.end, + Diagnostics.Cannot_find_module_0, + ["@angular/core"] + ) + ]; + const expectedRecursiveWatches = arrayToMap([`${tscWatch.projectRoot}`, `${tscWatch.projectRoot}/src`, `${tscWatch.projectRoot}/node_modules`, `${tscWatch.projectRoot}/node_modules/@types`], identity, () => 1); + verifyProject(); + verifyErrors(moduleNotFoundErr); + + let npmInstallComplete = false; + + // Simulate npm install + let filesAndFoldersToAdd: (File | Folder)[] = [ + { path: `${tscWatch.projectRoot}/node_modules` }, // This should queue update + { path: `${tscWatch.projectRoot}/node_modules/.staging` }, + { path: `${tscWatch.projectRoot}/node_modules/.staging/@babel` }, + { path: `${tscWatch.projectRoot}/node_modules/.staging/@babel/helper-plugin-utils-a06c629f` }, + { path: `${tscWatch.projectRoot}/node_modules/.staging/core-js-db53158d` }, + ]; + verifyWhileNpmInstall({ timeouts: 2, semantic: moduleNotFoundErr }); + + filesAndFoldersToAdd = [ + { path: `${tscWatch.projectRoot}/node_modules/.staging/@angular/platform-browser-dynamic-5efaaa1a` }, + { path: `${tscWatch.projectRoot}/node_modules/.staging/@angular/cli-c1e44b05/models/analytics.d.ts`, content: `export const x = 10;` }, + { path: `${tscWatch.projectRoot}/node_modules/.staging/@angular/core-0963aebf/index.d.ts`, content: `export const y = 10;` }, + ]; + // Since we added/removed in .staging no timeout + verifyWhileNpmInstall({ timeouts: 0, semantic: moduleNotFoundErr }); + + filesAndFoldersToAdd = []; + // Move things from staging to node_modules without triggering watch + const moduleFile: File = { + path: `${tscWatch.projectRoot}/node_modules/@angular/core/index.d.ts`, + content: `export const y = 10;` + }; + host.ensureFileOrFolder(moduleFile, /*ignoreWatchInvokedWithTriggerAsFileCreate*/ true, /*ignoreParentWatch*/ true); + // Since we added/removed in .staging no timeout + verifyWhileNpmInstall({ timeouts: 0, semantic: moduleNotFoundErr }); + + // Remove staging folder to remove errors + host.deleteFolder(`${tscWatch.projectRoot}/node_modules/.staging`, /*recursive*/ true); + npmInstallComplete = true; + projectFiles.push(moduleFile); + // Additional watch for watching script infos from node_modules + expectedRecursiveWatches.set(`${tscWatch.projectRoot}/node_modules`, 2); + verifyWhileNpmInstall({ timeouts: 2, semantic: [] }); + + function verifyWhileNpmInstall({ timeouts, semantic }: { timeouts: number; semantic: protocol.Diagnostic[] }) { + filesAndFoldersToAdd.forEach(f => host.ensureFileOrFolder(f)); + if (npmInstallComplete || timeoutDuringPartialInstallation) { + host.checkTimeoutQueueLengthAndRun(timeouts); + } + else { + host.checkTimeoutQueueLength(2); + } + verifyProject(); + verifyErrors(semantic, !npmInstallComplete && !timeoutDuringPartialInstallation ? 2 : undefined); + } + + function verifyProject() { + checkNumberOfConfiguredProjects(service, 1); + + const project = service.configuredProjects.get(config.path)!; + checkProjectActualFiles(project, map(projectFiles, f => f.path)); + + checkWatchedFilesDetailed(host, mapDefined(projectFiles, f => f === main || f === moduleFile ? undefined : f.path), 1); + checkWatchedDirectoriesDetailed(host, expectedRecursiveWatches, /*recursive*/ true); + checkWatchedDirectories(host, [], /*recursive*/ false); + } + + function verifyErrors(semantic: protocol.Diagnostic[], existingTimeouts?: number) { + verifyGetErrRequest({ + session, + host, + expected: [{ + file: main, + syntax: [], + semantic, + suggestion: [] + }], + existingTimeouts + }); + + } + } + + it("timeouts occur inbetween installation", () => { + verifyNpmInstall(/*timeoutDuringPartialInstallation*/ true); + }); + + it("timeout occurs after installation", () => { + verifyNpmInstall(/*timeoutDuringPartialInstallation*/ false); + }); + }); } From e9dbe1ca7a2f1bf516881ee3edb4636fb4621ced Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Mon, 6 Jan 2020 14:09:17 -0800 Subject: [PATCH 2/2] Use watch invoked with `node_modules/.staging` as watch for refreshing complete node_modules, so that npm install is reflected correctly Fixes #35966 --- src/compiler/resolutionCache.ts | 15 ++++++++++++--- src/compiler/watchPublic.ts | 5 +++-- src/server/editorServices.ts | 9 +++++---- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index 5af7919b6d620..050e8f5d858de 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -73,8 +73,15 @@ namespace ts { nonRecursive?: boolean; } - export function isPathIgnored(path: Path) { - return some(ignoredPaths, searchPath => stringContains(path, searchPath)); + export function removeIgnoredPath(path: Path): Path | undefined { + // Consider whole staging folder as if node_modules changed. + if (endsWith(path, "/node_modules/.staging")) { + return removeSuffix(path, "/.staging") as Path; + } + + return some(ignoredPaths, searchPath => stringContains(path, searchPath)) ? + undefined : + path; } /** @@ -722,7 +729,9 @@ namespace ts { } else { // If something to do with folder/file starting with "." in node_modules folder, skip it - if (isPathIgnored(fileOrDirectoryPath)) return false; + const updatedPath = removeIgnoredPath(fileOrDirectoryPath); + if (!updatedPath) return false; + fileOrDirectoryPath = updatedPath; // prevent saving an open file from over-eagerly triggering invalidation if (resolutionHost.fileIsOpen(fileOrDirectoryPath)) { diff --git a/src/compiler/watchPublic.ts b/src/compiler/watchPublic.ts index 3a8ef1debde2a..0ce0b216c674a 100644 --- a/src/compiler/watchPublic.ts +++ b/src/compiler/watchPublic.ts @@ -687,7 +687,7 @@ namespace ts { fileOrDirectory => { Debug.assert(!!configFileName); - const fileOrDirectoryPath = toPath(fileOrDirectory); + let fileOrDirectoryPath: Path | undefined = toPath(fileOrDirectory); // Since the file existance changed, update the sourceFiles cache if (cachedDirectoryStructureHost) { @@ -695,7 +695,8 @@ namespace ts { } nextSourceFileVersion(fileOrDirectoryPath); - if (isPathIgnored(fileOrDirectoryPath)) return; + fileOrDirectoryPath = removeIgnoredPath(fileOrDirectoryPath); + if (!fileOrDirectoryPath) return; // If the the added or created file or directory is not supported file name, ignore the file // But when watched directory is added/removed, we need to reload the file list diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 81d54197be2aa..7abc88ad8d393 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -1086,7 +1086,7 @@ namespace ts.server { this.host, directory, fileOrDirectory => { - const fileOrDirectoryPath = this.toPath(fileOrDirectory); + let fileOrDirectoryPath: Path | undefined = this.toPath(fileOrDirectory); const fsResult = project.getCachedDirectoryStructureHost().addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath); // don't trigger callback on open, existing files @@ -1097,7 +1097,8 @@ namespace ts.server { return; } - if (isPathIgnored(fileOrDirectoryPath)) return; + fileOrDirectoryPath = removeIgnoredPath(fileOrDirectoryPath); + if (!fileOrDirectoryPath) return; const configFilename = project.getConfigFilePath(); if (getBaseFileName(fileOrDirectoryPath) === "package.json" && !isInsideNodeModules(fileOrDirectoryPath) && @@ -2254,8 +2255,8 @@ namespace ts.server { this.host, watchDir, (fileOrDirectory) => { - const fileOrDirectoryPath = this.toPath(fileOrDirectory); - if (isPathIgnored(fileOrDirectoryPath)) return; + const fileOrDirectoryPath = removeIgnoredPath(this.toPath(fileOrDirectory)); + if (!fileOrDirectoryPath) return; // Has extension Debug.assert(result.refCount > 0);