Merge pull request #11993 from Microsoft/common_source_directory

Leave files from `node_modules` out when calculating `getCommonSourceDirectory`

Author: Andy

src/compiler/program.ts

@@ -431,13 +431,14 @@ namespace ts {
         return program;
 
         function getCommonSourceDirectory() {
-            if (typeof commonSourceDirectory === "undefined") {
-                if (options.rootDir && checkSourceFilesBelongToPath(files, options.rootDir)) {
+            if (commonSourceDirectory === undefined) {
+                const emittedFiles = filterSourceFilesInDirectory(files, isSourceFileFromExternalLibrary);
+                if (options.rootDir && checkSourceFilesBelongToPath(emittedFiles, options.rootDir)) {
                     // If a rootDir is specified and is valid use it as the commonSourceDirectory
                     commonSourceDirectory = getNormalizedAbsolutePath(options.rootDir, currentDirectory);
                 }
                 else {
-                    commonSourceDirectory = computeCommonSourceDirectory(files);
+                    commonSourceDirectory = computeCommonSourceDirectory(emittedFiles);
                 }
                 if (commonSourceDirectory && commonSourceDirectory[commonSourceDirectory.length - 1] !== directorySeparator) {
                     // Make sure directory path ends with directory separator so this string can directly

src/compiler/utilities.ts

@@ -2576,20 +2576,33 @@ namespace ts {
         }
         else {
             const sourceFiles = targetSourceFile === undefined ? getAllEmittableSourceFiles() : [targetSourceFile];
-            return filter(sourceFiles, isNonDeclarationFile);
+            return filterSourceFilesInDirectory(sourceFiles, file => host.isSourceFileFromExternalLibrary(file));
         }
 
         function getAllEmittableSourceFiles() {
             return options.noEmitForJsFiles ? filter(host.getSourceFiles(), sourceFile => !isSourceFileJavaScript(sourceFile)) : host.getSourceFiles();
         }
     }
 
+    /** Don't call this for `--outFile`, just for `--outDir` or plain emit. */
+    export function filterSourceFilesInDirectory(sourceFiles: SourceFile[], isSourceFileFromExternalLibrary: (file: SourceFile) => boolean): SourceFile[] {
+        return filter(sourceFiles, file => shouldEmitInDirectory(file, isSourceFileFromExternalLibrary));
+    }
+
     function isNonDeclarationFile(sourceFile: SourceFile) {
         return !isDeclarationFile(sourceFile);
     }
 
+    /**
+     * Whether a file should be emitted in a non-`--outFile` case.
+     * Don't emit if source file is a declaration file, or was located under node_modules
+     */
+    function shouldEmitInDirectory(sourceFile: SourceFile, isSourceFileFromExternalLibrary: (file: SourceFile) => boolean): boolean {
+        return isNonDeclarationFile(sourceFile) && !isSourceFileFromExternalLibrary(sourceFile);
+    }
+
     function isBundleEmitNonExternalModule(sourceFile: SourceFile) {
-        return !isDeclarationFile(sourceFile) && !isExternalModule(sourceFile);
+        return isNonDeclarationFile(sourceFile) && !isExternalModule(sourceFile);
     }
 
     /**
@@ -2677,8 +2690,7 @@ namespace ts {
         else {
             const sourceFiles = targetSourceFile === undefined ? getSourceFilesToEmit(host) : [targetSourceFile];
             for (const sourceFile of sourceFiles) {
-                // Don't emit if source file is a declaration file, or was located under node_modules
-                if (!isDeclarationFile(sourceFile) && !host.isSourceFileFromExternalLibrary(sourceFile)) {
+                if (shouldEmitInDirectory(sourceFile, file => host.isSourceFileFromExternalLibrary(file))) {
                     onSingleFileEmit(host, sourceFile);
                 }
             }

src/harness/compilerRunner.ts

@@ -170,7 +170,7 @@ class CompilerBaselineRunner extends RunnerBase {
             });
 
             it("Correct Sourcemap output for " + fileName, () => {
-                Harness.Compiler.doSourcemapBaseline(justName, options, result);
+                Harness.Compiler.doSourcemapBaseline(justName, options, result, harnessSettings);
             });
 
             it("Correct type/symbol baselines for " + fileName, () => {

src/harness/harness.ts

@@ -470,19 +470,6 @@ namespace Utils {
     }
 }
 
-namespace Harness.Path {
-    export function getFileName(fullPath: string) {
-        return fullPath.replace(/^.*[\\\/]/, "");
-    }
-
-    export function filePath(fullPath: string) {
-        fullPath = ts.normalizeSlashes(fullPath);
-        const components = fullPath.split("/");
-        const path: string[] = components.slice(0, components.length - 1);
-        return path.join("/") + "/";
-    }
-}
-
 namespace Harness {
     export interface IO {
         newLine(): string;
@@ -1090,7 +1077,9 @@ namespace Harness {
             { name: "suppressOutputPathCheck", type: "boolean" },
             { name: "noImplicitReferences", type: "boolean" },
             { name: "currentDirectory", type: "string" },
-            { name: "symlink", type: "string" }
+            { name: "symlink", type: "string" },
+            // Emitted js baseline will print full paths for every output file
+            { name: "fullEmitPaths", type: "boolean" }
         ];
 
         let optionsIndex: ts.Map<ts.CommandLineOption>;
@@ -1565,7 +1554,7 @@ namespace Harness {
             return file.writeByteOrderMark ? "\u00EF\u00BB\u00BF" : "";
         }
 
-        export function doSourcemapBaseline(baselinePath: string, options: ts.CompilerOptions, result: CompilerResult) {
+        export function doSourcemapBaseline(baselinePath: string, options: ts.CompilerOptions, result: CompilerResult, harnessSettings: Harness.TestCaseParser.CompilerSettings) {
             if (options.inlineSourceMap) {
                 if (result.sourceMaps.length > 0) {
                     throw new Error("No sourcemap files should be generated if inlineSourceMaps was set.");
@@ -1587,10 +1576,8 @@ namespace Harness {
                     }
 
                     let sourceMapCode = "";
-                    for (let i = 0; i < result.sourceMaps.length; i++) {
-                        sourceMapCode += "//// [" + Harness.Path.getFileName(result.sourceMaps[i].fileName) + "]\r\n";
-                        sourceMapCode += getByteOrderMarkText(result.sourceMaps[i]);
-                        sourceMapCode += result.sourceMaps[i].code;
+                    for (const sourceMap of result.sourceMaps) {
+                        sourceMapCode += fileOutput(sourceMap, harnessSettings);
                     }
 
                     return sourceMapCode;
@@ -1611,23 +1598,19 @@ namespace Harness {
                     tsCode += "//// [" + header + "] ////\r\n\r\n";
                 }
                 for (let i = 0; i < tsSources.length; i++) {
-                    tsCode += "//// [" + Harness.Path.getFileName(tsSources[i].unitName) + "]\r\n";
+                    tsCode += "//// [" + ts.getBaseFileName(tsSources[i].unitName) + "]\r\n";
                     tsCode += tsSources[i].content + (i < (tsSources.length - 1) ? "\r\n" : "");
                 }
 
                 let jsCode = "";
-                for (let i = 0; i < result.files.length; i++) {
-                    jsCode += "//// [" + Harness.Path.getFileName(result.files[i].fileName) + "]\r\n";
-                    jsCode += getByteOrderMarkText(result.files[i]);
-                    jsCode += result.files[i].code;
+                for (const file of result.files) {
+                    jsCode += fileOutput(file, harnessSettings);
                 }
 
                 if (result.declFilesCode.length > 0) {
                     jsCode += "\r\n\r\n";
-                    for (let i = 0; i < result.declFilesCode.length; i++) {
-                        jsCode += "//// [" + Harness.Path.getFileName(result.declFilesCode[i].fileName) + "]\r\n";
-                        jsCode += getByteOrderMarkText(result.declFilesCode[i]);
-                        jsCode += result.declFilesCode[i].code;
+                    for (const declFile of result.declFilesCode) {
+                        jsCode += fileOutput(declFile, harnessSettings);
                     }
                 }
 
@@ -1652,6 +1635,11 @@ namespace Harness {
             });
         }
 
+        function fileOutput(file: GeneratedFile, harnessSettings: Harness.TestCaseParser.CompilerSettings): string {
+            const fileName = harnessSettings["fullEmitPaths"] ? file.fileName : ts.getBaseFileName(file.fileName);
+            return "//// [" + fileName + "]\r\n" + getByteOrderMarkText(file) + file.code;
+        }
+
         export function collateOutputs(outputFiles: Harness.Compiler.GeneratedFile[]): string {
             // Collect, test, and sort the fileNames
             outputFiles.sort((a, b) => ts.compareStrings(cleanName(a.fileName), cleanName(b.fileName)));
@@ -1848,7 +1836,7 @@ namespace Harness {
             }
 
             // normalize the fileName for the single file case
-            currentFileName = testUnitData.length > 0 || currentFileName ? currentFileName : Path.getFileName(fileName);
+            currentFileName = testUnitData.length > 0 || currentFileName ? currentFileName : ts.getBaseFileName(fileName);
 
             // EOF, push whatever remains
             const newTestFile2 = {
@@ -2012,7 +2000,7 @@ namespace Harness {
 
     export function isDefaultLibraryFile(filePath: string): boolean {
         // We need to make sure that the filePath is prefixed with "lib." not just containing "lib." and end with ".d.ts"
-        const fileName = Path.getFileName(filePath);
+        const fileName = ts.getBaseFileName(filePath);
         return ts.startsWith(fileName, "lib.") && ts.endsWith(fileName, ".d.ts");
     }
 

src/harness/runnerbase.ts

@@ -32,7 +32,7 @@ abstract class RunnerBase {
     /** Replaces instances of full paths with fileNames only */
     static removeFullPaths(path: string) {
         // If its a full path (starts with "C:" or "/") replace with just the filename
-        let fixedPath = /^(\w:|\/)/.test(path) ? Harness.Path.getFileName(path) : path;
+        let fixedPath = /^(\w:|\/)/.test(path) ? ts.getBaseFileName(path) : path;
 
         // when running in the browser the 'full path' is the host name, shows up in error baselines
         const localHost = /http:\/localhost:\d+/g;

tests/baselines/reference/commonSourceDirectory.js

@@ -0,0 +1,29 @@
+//// [tests/cases/compiler/commonSourceDirectory.ts] ////
+
+//// [index.ts]
+// Test that importing a file from `node_modules` does not affect calculation of the common source directory.
+
+export const x = 0;
+
+//// [bar.d.ts]
+declare module "bar" {
+    export const y = 0;
+}
+
+//// [index.ts]
+/// <reference path="../types/bar.d.ts"/>
+import { x } from "foo";
+import { y } from "bar";
+x + y;
+
+
+//// [/app/bin/index.js]
+"use strict";
+/// <reference path="../types/bar.d.ts"/>
+var foo_1 = require("foo");
+var bar_1 = require("bar");
+foo_1.x + bar_1.y;
+//# sourceMappingURL=/app/myMapRoot/index.js.map
+
+//// [/app/bin/index.d.ts]
+/// <reference path="../../types/bar.d.ts" />

tests/baselines/reference/commonSourceDirectory.js.map

@@ -0,0 +1,58 @@
+===================================================================
+JsFile: index.js
+mapUrl: /app/myMapRoot/index.js.map
+sourceRoot: /app/mySourceRoot/
+sources: index.ts
+===================================================================
+-------------------------------------------------------------------
+emittedFile:/app/bin/index.js
+sourceFile:index.ts
+-------------------------------------------------------------------
+>>>"use strict";
+>>>/// <reference path="../types/bar.d.ts"/>
+1 >
+2 >^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+1 >
+2 >/// <reference path="../types/bar.d.ts"/>
+1 >Emitted(2, 1) Source(1, 1) + SourceIndex(0)
+2 >Emitted(2, 42) Source(1, 42) + SourceIndex(0)
+---
+>>>var foo_1 = require("foo");
+1 >
+2 >^^^^^^^^^^^^^^^^^^^^^^^^^^^
+3 >                           ^->
+1 >
+  >
+2 >import { x } from "foo";
+1 >Emitted(3, 1) Source(2, 1) + SourceIndex(0)
+2 >Emitted(3, 28) Source(2, 25) + SourceIndex(0)
+---
+>>>var bar_1 = require("bar");
+1->
+2 >^^^^^^^^^^^^^^^^^^^^^^^^^^^
+1->
+  >
+2 >import { y } from "bar";
+1->Emitted(4, 1) Source(3, 1) + SourceIndex(0)
+2 >Emitted(4, 28) Source(3, 25) + SourceIndex(0)
+---
+>>>foo_1.x + bar_1.y;
+1 >
+2 >^^^^^^^
+3 >       ^^^
+4 >          ^^^^^^^
+5 >                 ^
+6 >                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^->
+1 >
+  >
+2 >x
+3 >        + 
+4 >          y
+5 >                 ;
+1 >Emitted(5, 1) Source(4, 1) + SourceIndex(0)
+2 >Emitted(5, 8) Source(4, 2) + SourceIndex(0)
+3 >Emitted(5, 11) Source(4, 5) + SourceIndex(0)
+4 >Emitted(5, 18) Source(4, 6) + SourceIndex(0)
+5 >Emitted(5, 19) Source(4, 7) + SourceIndex(0)
+---
+>>>//# sourceMappingURL=/app/myMapRoot/index.js.map

tests/baselines/reference/commonSourceDirectory.sourcemap.txt

@@ -0,0 +1,24 @@
+=== /app/index.ts ===
+/// <reference path="../types/bar.d.ts"/>
+import { x } from "foo";
+>x : Symbol(x, Decl(index.ts, 1, 8))
+
+import { y } from "bar";
+>y : Symbol(y, Decl(index.ts, 2, 8))
+
+x + y;
+>x : Symbol(x, Decl(index.ts, 1, 8))
+>y : Symbol(y, Decl(index.ts, 2, 8))
+
+=== /node_modules/foo/index.ts ===
+// Test that importing a file from `node_modules` does not affect calculation of the common source directory.
+
+export const x = 0;
+>x : Symbol(x, Decl(index.ts, 2, 12))
+
+=== /types/bar.d.ts ===
+declare module "bar" {
+    export const y = 0;
+>y : Symbol(y, Decl(bar.d.ts, 1, 16))
+}
+

tests/baselines/reference/commonSourceDirectory.symbols

@@ -0,0 +1,27 @@
+=== /app/index.ts ===
+/// <reference path="../types/bar.d.ts"/>
+import { x } from "foo";
+>x : 0
+
+import { y } from "bar";
+>y : 0
+
+x + y;
+>x + y : number
+>x : 0
+>y : 0
+
+=== /node_modules/foo/index.ts ===
+// Test that importing a file from `node_modules` does not affect calculation of the common source directory.
+
+export const x = 0;
+>x : 0
+>0 : 0
+
+=== /types/bar.d.ts ===
+declare module "bar" {
+    export const y = 0;
+>y : 0
+>0 : 0
+}
+

tests/baselines/reference/commonSourceDirectory.types

@@ -0,0 +1,21 @@
+//// [tests/cases/compiler/commonSourceDirectory_dts.ts] ////
+
+//// [bar.d.ts]
+// Test that importing a file from `node_modules` does not affect calculation of the common source directory.
+
+declare const y: number;
+
+//// [index.ts]
+/// <reference path="../lib/bar.d.ts" />
+export const x = y;
+
+
+//// [/app/bin/index.js]
+"use strict";
+/// <reference path="../lib/bar.d.ts" />
+exports.x = y;
+//# sourceMappingURL=/app/myMapRoot/index.js.map
+
+//// [/app/bin/index.d.ts]
+/// <reference path="../lib/bar.d.ts" />
+export declare const x: number;