From 8c443b148179ebfe954944f8661731b401e388f9 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 30 May 2019 15:05:53 -0700 Subject: [PATCH 1/5] Stop invalidating resolution when file stays open --- src/compiler/resolutionCache.ts | 5 +++++ src/compiler/watch.ts | 1 + src/server/project.ts | 5 +++++ 3 files changed, 11 insertions(+) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index b82a5a78d7187..ff09efb05ea5e 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -54,6 +54,7 @@ namespace ts { writeLog(s: string): void; maxNumberOfFilesToIterateForInvalidation?: number; getCurrentProgram(): Program | undefined; + fileIsOpen(filePath: Path): boolean; } interface DirectoryWatchesOfFailedLookup { @@ -713,6 +714,10 @@ namespace ts { if (!isPathWithDefaultFailedLookupExtension(fileOrDirectoryPath) && !customFailedLookupPaths.has(fileOrDirectoryPath)) { return false; } + // prevent saving an open file from over-eagerly triggering invalidation + if (resolutionHost.fileIsOpen(fileOrDirectoryPath)) { + return; + } // Ignore emits from the program if (isEmittedFileOfProgram(resolutionHost.getCurrentProgram(), fileOrDirectoryPath)) { return false; diff --git a/src/compiler/watch.ts b/src/compiler/watch.ts index f9bf2a8468db6..0faf9f0c17581 100644 --- a/src/compiler/watch.ts +++ b/src/compiler/watch.ts @@ -691,6 +691,7 @@ namespace ts { hasChangedAutomaticTypeDirectiveNames = true; scheduleProgramUpdate(); }; + compilerHost.fileIsOpen = () => false; compilerHost.maxNumberOfFilesToIterateForInvalidation = host.maxNumberOfFilesToIterateForInvalidation; compilerHost.getCurrentProgram = getCurrentProgram; compilerHost.writeLog = writeLog; diff --git a/src/server/project.ts b/src/server/project.ts index 20cd5667dd9db..e10a0988f7185 100644 --- a/src/server/project.ts +++ b/src/server/project.ts @@ -457,6 +457,11 @@ namespace ts.server { return this.getTypeAcquisition().enable ? this.projectService.typingsInstaller.globalTypingsCacheLocation : undefined; } + /*@internal*/ + fileIsOpen(filePath: Path) { + return this.projectService.openFiles.has(filePath); + } + /*@internal*/ writeLog(s: string) { this.projectService.logger.info(s); From a30cacb562bf5cf8cd7ae8ade77f72421dddc58a Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 30 May 2019 16:56:27 -0700 Subject: [PATCH 2/5] Add test --- .../unittests/tsserver/resolutionCache.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/testRunner/unittests/tsserver/resolutionCache.ts b/src/testRunner/unittests/tsserver/resolutionCache.ts index 33b59e2faae3a..11b604bb20cb1 100644 --- a/src/testRunner/unittests/tsserver/resolutionCache.ts +++ b/src/testRunner/unittests/tsserver/resolutionCache.ts @@ -975,5 +975,33 @@ export const x = 10;` host.checkTimeoutQueueLength(0); }); }); + + describe("avoid unnecessary invalidation", () => { + it("failed lookup invalidation", () => { + const expectedNonRelativeDirectories = [`${projectLocation}/node_modules`, `${projectLocation}/src`]; + const module1Name = "module1"; + const module2Name = "module2"; + const fileContent = `import { module1 } from "${module1Name}";import { module2 } from "${module2Name}";`; + const file1: File = { + path: `${projectLocation}/src/file1.ts`, + content: fileContent + }; + const { module1, module2 } = getModules(`${projectLocation}/src/node_modules/module1/index.ts`, `${projectLocation}/node_modules/module2/index.ts`); + const files = [module1, module2, file1, configFile, libFile]; + const host = createServerHost(files); + const resolutionTrace = createHostModuleResolutionTrace(host); + const service = createProjectService(host); + service.openClientFile(file1.path); + const project = service.configuredProjects.get(configFile.path)!; + (project as ResolutionCacheHost).maxNumberOfFilesToIterateForInvalidation = 1; + const expectedTrace = getExpectedNonRelativeModuleResolutionTrace(host, file1, module1, module1Name); + getExpectedNonRelativeModuleResolutionTrace(host, file1, module2, module2Name, expectedTrace); + verifyTrace(resolutionTrace, expectedTrace); + verifyWatchesWithConfigFile(host, files, file1, expectedNonRelativeDirectories); + + host.invokeWatchedDirectoriesRecursiveCallback(projectLocation + "/src", "file1.ts"); + host.checkTimeoutQueueLengthAndRun(0); + }); + }); }); } From 6b92ccaffaa6ccd789b80f5e506c64cc9111a30d Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Thu, 30 May 2019 17:22:12 -0700 Subject: [PATCH 3/5] Respond to CR --- src/compiler/resolutionCache.ts | 2 +- src/compiler/watch.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index ff09efb05ea5e..6e0384219a1d8 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -716,7 +716,7 @@ namespace ts { } // prevent saving an open file from over-eagerly triggering invalidation if (resolutionHost.fileIsOpen(fileOrDirectoryPath)) { - return; + return false; } // Ignore emits from the program if (isEmittedFileOfProgram(resolutionHost.getCurrentProgram(), fileOrDirectoryPath)) { diff --git a/src/compiler/watch.ts b/src/compiler/watch.ts index 0faf9f0c17581..dbc78fa5d4e2e 100644 --- a/src/compiler/watch.ts +++ b/src/compiler/watch.ts @@ -691,7 +691,7 @@ namespace ts { hasChangedAutomaticTypeDirectiveNames = true; scheduleProgramUpdate(); }; - compilerHost.fileIsOpen = () => false; + compilerHost.fileIsOpen = returnFalse; compilerHost.maxNumberOfFilesToIterateForInvalidation = host.maxNumberOfFilesToIterateForInvalidation; compilerHost.getCurrentProgram = getCurrentProgram; compilerHost.writeLog = writeLog; From 7ac5fa783bd41ee5b64517f1ad85402ee0a49233 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 31 May 2019 11:24:54 -0700 Subject: [PATCH 4/5] Refactor and add wildcard scenario --- src/compiler/resolutionCache.ts | 9 +++++---- src/server/editorServices.ts | 8 +++++++- src/testRunner/unittests/tsserver/projects.ts | 20 +++++++++++++++++++ .../unittests/tsserver/resolutionCache.ts | 3 ++- 4 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/compiler/resolutionCache.ts b/src/compiler/resolutionCache.ts index 6e0384219a1d8..fb04d7b7a187e 100644 --- a/src/compiler/resolutionCache.ts +++ b/src/compiler/resolutionCache.ts @@ -699,6 +699,11 @@ namespace ts { // If something to do with folder/file starting with "." in node_modules folder, skip it if (isPathIgnored(fileOrDirectoryPath)) return false; + // prevent saving an open file from over-eagerly triggering invalidation + if (resolutionHost.fileIsOpen(fileOrDirectoryPath)) { + return false; + } + // Some file or directory in the watching directory is created // Return early if it does not have any of the watching extension or not the custom failed lookup path const dirOfFileOrDirectory = getDirectoryPath(fileOrDirectoryPath); @@ -714,10 +719,6 @@ namespace ts { if (!isPathWithDefaultFailedLookupExtension(fileOrDirectoryPath) && !customFailedLookupPaths.has(fileOrDirectoryPath)) { return false; } - // prevent saving an open file from over-eagerly triggering invalidation - if (resolutionHost.fileIsOpen(fileOrDirectoryPath)) { - return false; - } // Ignore emits from the program if (isEmittedFileOfProgram(resolutionHost.getCurrentProgram(), fileOrDirectoryPath)) { return false; diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index 6bfce902a049b..b0a6443263349 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -1002,7 +1002,13 @@ namespace ts.server { directory, fileOrDirectory => { const fileOrDirectoryPath = this.toPath(fileOrDirectory); - project.getCachedDirectoryStructureHost().addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath); + const fileSystemResult = project.getCachedDirectoryStructureHost().addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath); + + // don't trigger callback on open, existing files + if (project.fileIsOpen(fileOrDirectoryPath) && fileSystemResult && fileSystemResult.fileExists) { + return; + } + if (isPathIgnored(fileOrDirectoryPath)) return; const configFilename = project.getConfigFilePath(); diff --git a/src/testRunner/unittests/tsserver/projects.ts b/src/testRunner/unittests/tsserver/projects.ts index ba1eae236a5ec..dcbff895f1de3 100644 --- a/src/testRunner/unittests/tsserver/projects.ts +++ b/src/testRunner/unittests/tsserver/projects.ts @@ -1341,6 +1341,26 @@ var x = 10;` } }); + it("no project structure update on directory watch invoke on open file save", () => { + const projectRootPath = "/users/username/projects/project"; + const fileA: File = { + path: `${projectRootPath}/a.ts`, + content: "export const a = 10;" + }; + const config: File = { + path: `${projectRootPath}/tsconfig.json`, + content: "{}" + }; + const files = [fileA, config]; + const host = createServerHost(files); + const service = createProjectService(host); + service.openClientFile(fileA.path); + checkNumberOfProjects(service, { configuredProjects: 1 }); + + host.invokeWatchedDirectoriesRecursiveCallback(projectRootPath, "a.ts"); + host.checkTimeoutQueueLength(0); + }); + it("handles delayed directory watch invoke on file creation", () => { const projectRootPath = "/users/username/projects/project"; const fileB: File = { diff --git a/src/testRunner/unittests/tsserver/resolutionCache.ts b/src/testRunner/unittests/tsserver/resolutionCache.ts index 11b604bb20cb1..05cbf8b93885d 100644 --- a/src/testRunner/unittests/tsserver/resolutionCache.ts +++ b/src/testRunner/unittests/tsserver/resolutionCache.ts @@ -977,7 +977,7 @@ export const x = 10;` }); describe("avoid unnecessary invalidation", () => { - it("failed lookup invalidation", () => { + it("unnecessary lookup invalidation on save", () => { const expectedNonRelativeDirectories = [`${projectLocation}/node_modules`, `${projectLocation}/src`]; const module1Name = "module1"; const module2Name = "module2"; @@ -999,6 +999,7 @@ export const x = 10;` verifyTrace(resolutionTrace, expectedTrace); verifyWatchesWithConfigFile(host, files, file1, expectedNonRelativeDirectories); + // invoke callback to simulate saving host.invokeWatchedDirectoriesRecursiveCallback(projectLocation + "/src", "file1.ts"); host.checkTimeoutQueueLengthAndRun(0); }); From d3b6ba557ac97837ed1c419887dec39cc9acb834 Mon Sep 17 00:00:00 2001 From: Benjamin Lichtman Date: Fri, 31 May 2019 14:46:15 -0700 Subject: [PATCH 5/5] Change test instead of behavior --- src/server/editorServices.ts | 4 ++-- src/testRunner/unittests/tsserver/projects.ts | 8 ++++---- src/testRunner/unittests/tsserver/resolutionCache.ts | 2 +- src/testRunner/unittests/tsserver/syntaxOperations.ts | 5 +++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/server/editorServices.ts b/src/server/editorServices.ts index b0a6443263349..b868286ea51af 100644 --- a/src/server/editorServices.ts +++ b/src/server/editorServices.ts @@ -1002,10 +1002,10 @@ namespace ts.server { directory, fileOrDirectory => { const fileOrDirectoryPath = this.toPath(fileOrDirectory); - const fileSystemResult = project.getCachedDirectoryStructureHost().addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath); + project.getCachedDirectoryStructureHost().addOrDeleteFileOrDirectory(fileOrDirectory, fileOrDirectoryPath); // don't trigger callback on open, existing files - if (project.fileIsOpen(fileOrDirectoryPath) && fileSystemResult && fileSystemResult.fileExists) { + if (project.fileIsOpen(fileOrDirectoryPath)) { return; } diff --git a/src/testRunner/unittests/tsserver/projects.ts b/src/testRunner/unittests/tsserver/projects.ts index dcbff895f1de3..409ce5c525f74 100644 --- a/src/testRunner/unittests/tsserver/projects.ts +++ b/src/testRunner/unittests/tsserver/projects.ts @@ -1343,7 +1343,7 @@ var x = 10;` it("no project structure update on directory watch invoke on open file save", () => { const projectRootPath = "/users/username/projects/project"; - const fileA: File = { + const file1: File = { path: `${projectRootPath}/a.ts`, content: "export const a = 10;" }; @@ -1351,13 +1351,13 @@ var x = 10;` path: `${projectRootPath}/tsconfig.json`, content: "{}" }; - const files = [fileA, config]; + const files = [file1, config]; const host = createServerHost(files); const service = createProjectService(host); - service.openClientFile(fileA.path); + service.openClientFile(file1.path); checkNumberOfProjects(service, { configuredProjects: 1 }); - host.invokeWatchedDirectoriesRecursiveCallback(projectRootPath, "a.ts"); + host.modifyFile(file1.path, file1.content, { invokeFileDeleteCreateAsPartInsteadOfChange: true }); host.checkTimeoutQueueLength(0); }); diff --git a/src/testRunner/unittests/tsserver/resolutionCache.ts b/src/testRunner/unittests/tsserver/resolutionCache.ts index 05cbf8b93885d..768111f3bfeb4 100644 --- a/src/testRunner/unittests/tsserver/resolutionCache.ts +++ b/src/testRunner/unittests/tsserver/resolutionCache.ts @@ -1000,7 +1000,7 @@ export const x = 10;` verifyWatchesWithConfigFile(host, files, file1, expectedNonRelativeDirectories); // invoke callback to simulate saving - host.invokeWatchedDirectoriesRecursiveCallback(projectLocation + "/src", "file1.ts"); + host.modifyFile(file1.path, file1.content, { invokeFileDeleteCreateAsPartInsteadOfChange: true }); host.checkTimeoutQueueLengthAndRun(0); }); }); diff --git a/src/testRunner/unittests/tsserver/syntaxOperations.ts b/src/testRunner/unittests/tsserver/syntaxOperations.ts index 66a962d362507..d3127355191cf 100644 --- a/src/testRunner/unittests/tsserver/syntaxOperations.ts +++ b/src/testRunner/unittests/tsserver/syntaxOperations.ts @@ -60,13 +60,14 @@ describe("Test Suite 1", () => { const navBarResultUnitTest1 = navBarFull(session, unitTest1); host.deleteFile(unitTest1.path); - host.checkTimeoutQueueLengthAndRun(2); - checkProjectActualFiles(project, expectedFilesWithoutUnitTest1); + host.checkTimeoutQueueLengthAndRun(0); + checkProjectActualFiles(project, expectedFilesWithUnitTest1); session.executeCommandSeq({ command: protocol.CommandTypes.Close, arguments: { file: unitTest1.path } }); + host.checkTimeoutQueueLengthAndRun(2); checkProjectActualFiles(project, expectedFilesWithoutUnitTest1); const unitTest1WithChangedContent: File = {