Merge pull request #19688 from Microsoft/npmInstallAtTypes

Handle cases when npm install doesnt get triggered with actual failed lookup location but instead the trigger is some folder in the node_modules

Author: sheetalkamat

src/compiler/resolutionCache.ts

@@ -320,6 +320,10 @@ namespace ts {
             return endsWith(dirPath, "/node_modules");
         }
 
+        function isNodeModulesAtTypesDirectory(dirPath: Path) {
+            return endsWith(dirPath, "/node_modules/@types");
+        }
+
         function isDirectoryAtleastAtLevelFromFSRoot(dirPath: Path, minLevels: number) {
             for (let searchIndex = getRootLength(dirPath); minLevels > 0; minLevels--) {
                 searchIndex = dirPath.indexOf(directorySeparator, searchIndex) + 1;
@@ -560,11 +564,21 @@ namespace ts {
             else {
                 // 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
-                if (!isPathWithDefaultFailedLookupExtension(fileOrDirectoryPath) && !customFailedLookupPaths.has(fileOrDirectoryPath)) {
-                    return false;
+                const dirOfFileOrDirectory = getDirectoryPath(fileOrDirectoryPath);
+                if (isNodeModulesAtTypesDirectory(dirOfFileOrDirectory) || isNodeModulesDirectory(dirOfFileOrDirectory)) {
+                    // Invalidate any resolution from this directory
+                    isChangedFailedLookupLocation = location => {
+                        const locationPath = resolutionHost.toPath(location);
+                        return locationPath === fileOrDirectoryPath || startsWith(resolutionHost.toPath(location), fileOrDirectoryPath);
+                    };
+                }
+                else {
+                    if (!isPathWithDefaultFailedLookupExtension(fileOrDirectoryPath) && !customFailedLookupPaths.has(fileOrDirectoryPath)) {
+                        return false;
+                    }
+                    // Resolution need to be invalidated if failed lookup location is same as the file or directory getting created
+                    isChangedFailedLookupLocation = location => resolutionHost.toPath(location) === fileOrDirectoryPath;
                 }
-                // Resolution need to be invalidated if failed lookup location is same as the file or directory getting created
-                isChangedFailedLookupLocation = location => resolutionHost.toPath(location) === fileOrDirectoryPath;
             }
             const hasChangedFailedLookupLocation = (resolution: ResolutionWithFailedLookupLocations) => some(resolution.failedLookupLocations, isChangedFailedLookupLocation);
             const invalidatedFilesCount = filesWithInvalidatedResolutions && filesWithInvalidatedResolutions.size;

src/harness/unittests/tscWatchMode.ts

@@ -1988,7 +1988,7 @@ declare module "fs" {
 
             checkProgramActualFiles(watch(), mapDefined(files, f => f === configFile ? undefined : f.path));
             file1.content = "var zz30 = 100;";
-            host.reloadFS(files, /*invokeDirectoryWatcherInsteadOfFileChanged*/ true);
+            host.reloadFS(files, { invokeDirectoryWatcherInsteadOfFileChanged: true });
             host.runQueuedTimeoutCallbacks();
 
             checkProgramActualFiles(watch(), mapDefined(files, f => f === configFile ? undefined : f.path));

src/harness/unittests/tsserverProjectSystem.ts

@@ -3445,6 +3445,117 @@ namespace ts.projectSystem {
             diags = session.executeCommand(getErrRequest).response as server.protocol.Diagnostic[];
             verifyNoDiagnostics(diags);
         });
+
+        function assertEvent(actualOutput: string, expectedEvent: protocol.Event, host: TestServerHost) {
+            assert.equal(actualOutput, server.formatMessage(expectedEvent, nullLogger, Utils.byteLength, host.newLine));
+        }
+
+        function checkErrorMessage(host: TestServerHost, eventName: "syntaxDiag" | "semanticDiag", diagnostics: protocol.DiagnosticEventBody) {
+            const outputs = host.getOutput();
+            assert.isTrue(outputs.length >= 1, outputs.toString());
+            const event: protocol.Event = {
+                seq: 0,
+                type: "event",
+                event: eventName,
+                body: diagnostics
+            };
+            assertEvent(outputs[0], event, host);
+        }
+
+        function checkCompleteEvent(host: TestServerHost, numberOfCurrentEvents: number, expectedSequenceId: number) {
+            const outputs = host.getOutput();
+            assert.equal(outputs.length, numberOfCurrentEvents, outputs.toString());
+            const event: protocol.RequestCompletedEvent = {
+                seq: 0,
+                type: "event",
+                event: "requestCompleted",
+                body: {
+                    request_seq: expectedSequenceId
+                }
+            };
+            assertEvent(outputs[numberOfCurrentEvents - 1], event, host);
+        }
+
+        function checkProjectUpdatedInBackgroundEvent(host: TestServerHost, openFiles: string[]) {
+            const outputs = host.getOutput();
+            assert.equal(outputs.length, 1, outputs.toString());
+            const event: protocol.ProjectsUpdatedInBackgroundEvent = {
+                seq: 0,
+                type: "event",
+                event: "projectsUpdatedInBackground",
+                body: {
+                    openFiles
+                }
+            };
+            assertEvent(outputs[0], event, host);
+        }
+
+        it("npm install @types works", () => {
+            const folderPath = "/a/b/projects/temp";
+            const file1: FileOrFolder = {
+                path: `${folderPath}/a.ts`,
+                content: 'import f = require("pad")'
+            };
+            const files = [file1, libFile];
+            const host = createServerHost(files);
+            const session = createSession(host, { canUseEvents: true });
+            const service = session.getProjectService();
+            session.executeCommandSeq<protocol.OpenRequest>({
+                command: server.CommandNames.Open,
+                arguments: {
+                    file: file1.path,
+                    fileContent: file1.content,
+                    scriptKindName: "TS",
+                    projectRootPath: folderPath
+                }
+            });
+            checkNumberOfProjects(service, { inferredProjects: 1 });
+            host.clearOutput();
+            const expectedSequenceId = session.getNextSeq();
+            session.executeCommandSeq<protocol.GeterrRequest>({
+                command: server.CommandNames.Geterr,
+                arguments: {
+                    delay: 0,
+                    files: [file1.path]
+                }
+            });
+
+            host.checkTimeoutQueueLengthAndRun(1);
+            checkErrorMessage(host, "syntaxDiag", { file: file1.path, diagnostics: [] });
+            host.clearOutput();
+
+            host.runQueuedImmediateCallbacks();
+            const moduleNotFound = Diagnostics.Cannot_find_module_0;
+            const startOffset = file1.content.indexOf('"') + 1;
+            checkErrorMessage(host, "semanticDiag", {
+                file: file1.path, diagnostics: [{
+                    start: { line: 1, offset: startOffset },
+                    end: { line: 1, offset: startOffset + '"pad"'.length },
+                    text: formatStringFromArgs(moduleNotFound.message, ["pad"]),
+                    code: moduleNotFound.code,
+                    category: DiagnosticCategory[moduleNotFound.category].toLowerCase()
+                }]
+            });
+            checkCompleteEvent(host, 2, expectedSequenceId);
+            host.clearOutput();
+
+            const padIndex: FileOrFolder = {
+                path: `${folderPath}/node_modules/@types/pad/index.d.ts`,
+                content: "export = pad;declare function pad(length: number, text: string, char ?: string): string;"
+            };
+            files.push(padIndex);
+            host.reloadFS(files, { ignoreWatchInvokedWithTriggerAsFileCreate: true });
+            host.runQueuedTimeoutCallbacks();
+            checkProjectUpdatedInBackgroundEvent(host, [file1.path]);
+            host.clearOutput();
+
+            host.runQueuedTimeoutCallbacks();
+            checkErrorMessage(host, "syntaxDiag", { file: file1.path, diagnostics: [] });
+            host.clearOutput();
+
+            host.runQueuedImmediateCallbacks();
+            checkErrorMessage(host, "semanticDiag", { file: file1.path, diagnostics: [] });
+        });
     });
 
     describe("Configure file diagnostics events", () => {

src/harness/virtualFileSystemWithWatch.ts

@@ -226,6 +226,11 @@ interface Array<T> {}`
         directoryName: string;
     }
 
+    export interface ReloadWatchInvokeOptions {
+        invokeDirectoryWatcherInsteadOfFileChanged: boolean;
+        ignoreWatchInvokedWithTriggerAsFileCreate: boolean;
+    }
+
     export class TestServerHost implements server.ServerHost, FormatDiagnosticsHost {
         args: string[] = [];
 
@@ -270,7 +275,7 @@ interface Array<T> {}`
             return s;
         }
 
-        reloadFS(fileOrFolderList: ReadonlyArray<FileOrFolder>, invokeDirectoryWatcherInsteadOfFileChanged?: boolean) {
+        reloadFS(fileOrFolderList: ReadonlyArray<FileOrFolder>, options?: Partial<ReloadWatchInvokeOptions>) {
             const mapNewLeaves = createMap<true>();
             const isNewFs = this.fs.size === 0;
             fileOrFolderList = fileOrFolderList.concat(this.withSafeList ? safeList : []);
@@ -291,7 +296,7 @@ interface Array<T> {}`
                             // Update file
                             if (currentEntry.content !== fileOrDirectory.content) {
                                 currentEntry.content = fileOrDirectory.content;
-                                if (invokeDirectoryWatcherInsteadOfFileChanged) {
+                                if (options && options.invokeDirectoryWatcherInsteadOfFileChanged) {
                                     this.invokeDirectoryWatcher(getDirectoryPath(currentEntry.fullPath), currentEntry.fullPath);
                                 }
                                 else {
@@ -314,7 +319,7 @@ interface Array<T> {}`
                     }
                 }
                 else {
-                    this.ensureFileOrFolder(fileOrDirectory);
+                    this.ensureFileOrFolder(fileOrDirectory, options && options.ignoreWatchInvokedWithTriggerAsFileCreate);
                 }
             }
 
@@ -331,12 +336,12 @@ interface Array<T> {}`
             }
         }
 
-        ensureFileOrFolder(fileOrDirectory: FileOrFolder) {
+        ensureFileOrFolder(fileOrDirectory: FileOrFolder, ignoreWatchInvokedWithTriggerAsFileCreate?: boolean) {
             if (isString(fileOrDirectory.content)) {
                 const file = this.toFile(fileOrDirectory);
                 Debug.assert(!this.fs.get(file.path));
                 const baseFolder = this.ensureFolder(getDirectoryPath(file.fullPath));
-                this.addFileOrFolderInFolder(baseFolder, file);
+                this.addFileOrFolderInFolder(baseFolder, file, ignoreWatchInvokedWithTriggerAsFileCreate);
             }
             else {
                 const fullPath = getNormalizedAbsolutePath(fileOrDirectory.path, this.currentDirectory);
@@ -365,10 +370,13 @@ interface Array<T> {}`
             return folder;
         }
 
-        private addFileOrFolderInFolder(folder: Folder, fileOrDirectory: File | Folder) {
+        private addFileOrFolderInFolder(folder: Folder, fileOrDirectory: File | Folder, ignoreWatch?: boolean) {
             folder.entries.push(fileOrDirectory);
             this.fs.set(fileOrDirectory.path, fileOrDirectory);
 
+            if (ignoreWatch) {
+                return;
+            }
             if (isFile(fileOrDirectory)) {
                 this.invokeFileWatcher(fileOrDirectory.fullPath, FileWatcherEventKind.Created);
             }