Skip to content

Do not watch tsconfig files from folders that we canot watch (because they are at roots we want to ignore) #31818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 35 additions & 35 deletions src/compiler/resolutionCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,41 @@ namespace ts {
return some(ignoredPaths, searchPath => stringContains(path, searchPath));
}

/**
* Filter out paths like
* "/", "/user", "/user/username", "/user/username/folderAtRoot",
* "c:/", "c:/users", "c:/users/username", "c:/users/username/folderAtRoot", "c:/folderAtRoot"
* @param dirPath
*/
export function canWatchDirectory(dirPath: Path) {
const rootLength = getRootLength(dirPath);
if (dirPath.length === rootLength) {
// Ignore "/", "c:/"
return false;
}

const nextDirectorySeparator = dirPath.indexOf(directorySeparator, rootLength);
if (nextDirectorySeparator === -1) {
// ignore "/user", "c:/users" or "c:/folderAtRoot"
return false;
}

if (dirPath.charCodeAt(0) !== CharacterCodes.slash &&
dirPath.substr(rootLength, nextDirectorySeparator).search(/users/i) === -1) {
// Paths like c:/folderAtRoot/subFolder are allowed
return true;
}

for (let searchIndex = nextDirectorySeparator + 1, searchLevels = 2; searchLevels > 0; searchLevels--) {
searchIndex = dirPath.indexOf(directorySeparator, searchIndex) + 1;
if (searchIndex === 0) {
// Folder isnt at expected minimun levels
return false;
}
}
return true;
}

export const maxNumberOfFilesToIterateForInvalidation = 256;

type GetResolutionWithResolvedFileName<T extends ResolutionWithFailedLookupLocations = ResolutionWithFailedLookupLocations, R extends ResolutionWithResolvedFileName = ResolutionWithResolvedFileName> =
Expand Down Expand Up @@ -373,41 +408,6 @@ namespace ts {
return endsWith(dirPath, "/node_modules/@types");
}

/**
* Filter out paths like
* "/", "/user", "/user/username", "/user/username/folderAtRoot",
* "c:/", "c:/users", "c:/users/username", "c:/users/username/folderAtRoot", "c:/folderAtRoot"
* @param dirPath
*/
function canWatchDirectory(dirPath: Path) {
const rootLength = getRootLength(dirPath);
if (dirPath.length === rootLength) {
// Ignore "/", "c:/"
return false;
}

const nextDirectorySeparator = dirPath.indexOf(directorySeparator, rootLength);
if (nextDirectorySeparator === -1) {
// ignore "/user", "c:/users" or "c:/folderAtRoot"
return false;
}

if (dirPath.charCodeAt(0) !== CharacterCodes.slash &&
dirPath.substr(rootLength, nextDirectorySeparator).search(/users/i) === -1) {
// Paths like c:/folderAtRoot/subFolder are allowed
return true;
}

for (let searchIndex = nextDirectorySeparator + 1, searchLevels = 2; searchLevels > 0; searchLevels--) {
searchIndex = dirPath.indexOf(directorySeparator, searchIndex) + 1;
if (searchIndex === 0) {
// Folder isnt at expected minimun levels
return false;
}
}
return true;
}

function getDirectoryToWatchFailedLookupLocation(failedLookupLocation: string, failedLookupLocationPath: Path): DirectoryOfFailedLookupWatch | undefined {
if (isInDirectoryPath(rootPath, failedLookupLocationPath)) {
// Ensure failed look up is normalized path
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/watch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ namespace ts {
return ExitStatus.Success;
}

const noopFileWatcher: FileWatcher = { close: noop };
export const noopFileWatcher: FileWatcher = { close: noop };

export function createWatchHost(system = sys, reportWatchStatus?: WatchStatusReporter): WatchHost {
const onWatchStatusChange = reportWatchStatus || createWatchStatusReporter(system);
Expand Down
23 changes: 15 additions & 8 deletions src/server/editorServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1333,7 +1333,11 @@ namespace ts.server {

const watches: WatchType[] = [];
if (configFileExistenceInfo.configFileWatcherForRootOfInferredProject) {
watches.push(WatchType.ConfigFileForInferredRoot);
watches.push(
configFileExistenceInfo.configFileWatcherForRootOfInferredProject === noopFileWatcher ?
WatchType.NoopConfigFileForInferredRoot :
WatchType.ConfigFileForInferredRoot
);
}
if (this.configuredProjects.has(canonicalConfigFilePath)) {
watches.push(WatchType.ConfigFile);
Expand All @@ -1349,13 +1353,16 @@ namespace ts.server {
canonicalConfigFilePath: string,
configFileExistenceInfo: ConfigFileExistenceInfo
) {
configFileExistenceInfo.configFileWatcherForRootOfInferredProject = this.watchFactory.watchFile(
this.host,
configFileName,
(_filename, eventKind) => this.onConfigFileChangeForOpenScriptInfo(configFileName, eventKind),
PollingInterval.High,
WatchType.ConfigFileForInferredRoot
);
configFileExistenceInfo.configFileWatcherForRootOfInferredProject =
canWatchDirectory(getDirectoryPath(canonicalConfigFilePath) as Path) ?
this.watchFactory.watchFile(
this.host,
configFileName,
(_filename, eventKind) => this.onConfigFileChangeForOpenScriptInfo(configFileName, eventKind),
PollingInterval.High,
WatchType.ConfigFileForInferredRoot
) :
noopFileWatcher;
this.logConfigFileWatchUpdate(configFileName, canonicalConfigFilePath, configFileExistenceInfo, ConfigFileWatcherStatus.UpdatedCallback);
}

Expand Down
1 change: 1 addition & 0 deletions src/server/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,5 +226,6 @@ namespace ts {
ConfigFileForInferredRoot = "Config file for the inferred project root",
NodeModulesForClosedScriptInfo = "node_modules for closed script infos in them",
MissingSourceMapFile = "Missing source map file",
NoopConfigFileForInferredRoot = "Noop Config file for the inferred project root",
}
}
26 changes: 21 additions & 5 deletions src/testRunner/unittests/tsserver/configFileSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,7 @@ namespace ts.projectSystem {
const project = projectService.inferredProjects[0];
assert.isDefined(project);

const filesToWatch = [libFile.path];
forEachAncestorDirectory(dirOfFile, ancestor => {
filesToWatch.push(combinePaths(ancestor, "tsconfig.json"));
filesToWatch.push(combinePaths(ancestor, "jsconfig.json"));
});
const filesToWatch = [libFile.path, ...getConfigFilesToWatch(dirOfFile)];

checkProjectActualFiles(project, [file.path, libFile.path]);
checkWatchedFiles(host, filesToWatch);
Expand Down Expand Up @@ -170,5 +166,25 @@ namespace ts.projectSystem {
verifyInferredProject(host, projectService);
});
});

describe("should not search and watch config files from directories that cannot be watched", () => {
const root = "/root/teams/VSCode68/Shared Documents/General/jt-ts-test-workspace";
function verifyConfigFileWatch(projectRootPath: string | undefined) {
const path = `${root}/x.js`;
const host = createServerHost([libFile, { path, content: "const x = 10" }], { useCaseSensitiveFileNames: true });
const service = createProjectService(host);
service.openClientFile(path, /*fileContent*/ undefined, /*scriptKind*/ undefined, projectRootPath);
checkNumberOfProjects(service, { inferredProjects: 1 });
checkProjectActualFiles(service.inferredProjects[0], [path, libFile.path]);
checkWatchedFilesDetailed(host, [libFile.path, ...getConfigFilesToWatch(root)], 1);
}

it("when projectRootPath is not present", () => {
verifyConfigFileWatch(/*projectRootPath*/ undefined);
});
it("when projectRootPath is present but file is not from project root", () => {
verifyConfigFileWatch("/a/b");
});
});
});
}
24 changes: 17 additions & 7 deletions src/testRunner/unittests/tsserver/configuredProjects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,22 @@ namespace ts.projectSystem {
});

it("add and then remove a config file in a folder with loose files", () => {
const projectRoot = "/user/username/projects/project";
const configFile: File = {
path: "/a/b/tsconfig.json",
path: `${projectRoot}/tsconfig.json`,
content: `{
"files": ["commonFile1.ts"]
}`
};
const commonFile1: File = {
path: `${projectRoot}/commonFile1.ts`,
content: "let x = 1"
};
const commonFile2: File = {
path: `${projectRoot}/commonFile2.ts`,
content: "let y = 1"
};

const filesWithoutConfig = [libFile, commonFile1, commonFile2];
const host = createServerHost(filesWithoutConfig);

Expand All @@ -100,8 +110,7 @@ namespace ts.projectSystem {
checkProjectActualFiles(projectService.inferredProjects[0], [commonFile1.path, libFile.path]);
checkProjectActualFiles(projectService.inferredProjects[1], [commonFile2.path, libFile.path]);

const configFileLocations = ["/", "/a/", "/a/b/"];
const watchedFiles = flatMap(configFileLocations, location => [location + "tsconfig.json", location + "jsconfig.json"]).concat(libFile.path);
const watchedFiles = getConfigFilesToWatch(projectRoot).concat(libFile.path);
checkWatchedFiles(host, watchedFiles);

// Add a tsconfig file
Expand Down Expand Up @@ -431,20 +440,21 @@ namespace ts.projectSystem {
});

it("open file become a part of configured project if it is referenced from root file", () => {
const projectRoot = "/user/username/projects/project";
const file1 = {
path: "/a/b/f1.ts",
path: `${projectRoot}/a/b/f1.ts`,
content: "export let x = 5"
};
const file2 = {
path: "/a/c/f2.ts",
path: `${projectRoot}/a/c/f2.ts`,
content: `import {x} from "../b/f1"`
};
const file3 = {
path: "/a/c/f3.ts",
path: `${projectRoot}/a/c/f3.ts`,
content: "export let y = 1"
};
const configFile = {
path: "/a/c/tsconfig.json",
path: `${projectRoot}/a/c/tsconfig.json`,
content: JSON.stringify({ compilerOptions: {}, files: ["f2.ts", "f3.ts"] })
};

Expand Down
7 changes: 7 additions & 0 deletions src/testRunner/unittests/tsserver/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,13 @@ namespace ts.projectSystem {
return getRootsToWatchWithAncestorDirectory(currentDirectory, nodeModulesAtTypes);
}

export function getConfigFilesToWatch(folder: string) {
return [
...getRootsToWatchWithAncestorDirectory(folder, "tsconfig.json"),
...getRootsToWatchWithAncestorDirectory(folder, "jsconfig.json")
];
}

export function checkOpenFiles(projectService: server.ProjectService, expectedFiles: File[]) {
checkArray("Open files", arrayFrom(projectService.openFiles.keys(), path => projectService.getScriptInfoForPath(path as Path)!.fileName), expectedFiles.map(file => file.path));
}
Expand Down
22 changes: 11 additions & 11 deletions src/testRunner/unittests/tsserver/inferredProjects.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
namespace ts.projectSystem {
describe("unittests:: tsserver:: Inferred projects", () => {
it("create inferred project", () => {
const projectRoot = "/user/username/projects/project";
const appFile: File = {
path: "/a/b/c/app.ts",
path: `${projectRoot}/app.ts`,
content: `
import {f} from "./module"
console.log(f)
`
};

const moduleFile: File = {
path: "/a/b/c/module.d.ts",
path: `${projectRoot}/module.d.ts`,
content: `export let x: number`
};
const host = createServerHost([appFile, moduleFile, libFile]);
Expand All @@ -24,20 +25,19 @@ namespace ts.projectSystem {
const project = projectService.inferredProjects[0];

checkArray("inferred project", project.getFileNames(), [appFile.path, libFile.path, moduleFile.path]);
const configFileLocations = ["/a/b/c/", "/a/b/", "/a/", "/"];
const configFiles = flatMap(configFileLocations, location => [location + "tsconfig.json", location + "jsconfig.json"]);
checkWatchedFiles(host, configFiles.concat(libFile.path, moduleFile.path));
checkWatchedDirectories(host, ["/a/b/c"], /*recursive*/ false);
checkWatchedDirectories(host, [combinePaths(getDirectoryPath(appFile.path), nodeModulesAtTypes)], /*recursive*/ true);
checkWatchedFiles(host, getConfigFilesToWatch(projectRoot).concat(libFile.path, moduleFile.path));
checkWatchedDirectories(host, [projectRoot], /*recursive*/ false);
checkWatchedDirectories(host, [combinePaths(projectRoot, nodeModulesAtTypes)], /*recursive*/ true);
});

it("should use only one inferred project if 'useOneInferredProject' is set", () => {
const projectRoot = "/user/username/projects/project";
const file1 = {
path: "/a/b/main.ts",
path: `${projectRoot}/a/b/main.ts`,
content: "let x =1;"
};
const configFile: File = {
path: "/a/b/tsconfig.json",
path: `${projectRoot}/a/b/tsconfig.json`,
content: `{
"compilerOptions": {
"target": "es6"
Expand All @@ -46,12 +46,12 @@ namespace ts.projectSystem {
}`
};
const file2 = {
path: "/a/c/main.ts",
path: `${projectRoot}/a/c/main.ts`,
content: "let x =1;"
};

const file3 = {
path: "/a/d/main.ts",
path: `${projectRoot}/a/d/main.ts`,
content: "let x =1;"
};

Expand Down
6 changes: 2 additions & 4 deletions src/testRunner/unittests/tsserver/resolutionCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -818,10 +818,8 @@ namespace ts.projectSystem {
verifyTrace(resolutionTrace, expectedTrace);

const currentDirectory = getDirectoryPath(file1.path);
const watchedFiles = mapDefined(files, f => f === file1 || f.path.indexOf("/node_modules/") !== -1 ? undefined : f.path);
forEachAncestorDirectory(currentDirectory, d => {
watchedFiles.push(combinePaths(d, "tsconfig.json"), combinePaths(d, "jsconfig.json"));
});
const watchedFiles = mapDefined(files, f => f === file1 || f.path.indexOf("/node_modules/") !== -1 ? undefined : f.path)
.concat(getConfigFilesToWatch(`${projectLocation}/product/src`));
const watchedRecursiveDirectories = getTypeRootsFromLocation(currentDirectory).concat([
`${currentDirectory}/node_modules`, `${currentDirectory}/feature`, `${projectLocation}/product/${nodeModules}`,
`${projectLocation}/${nodeModules}`, `${projectLocation}/product/test/${nodeModules}`,
Expand Down
18 changes: 13 additions & 5 deletions src/testRunner/unittests/tsserver/typingsInstaller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -986,14 +986,15 @@ namespace ts.projectSystem {
});

it("should redo resolution that resolved to '.js' file after typings are installed", () => {
const projects = `/user/username/projects`;
const file: TestFSWithWatch.File = {
path: "/a/b/app.js",
path: `${projects}/a/b/app.js`,
content: `
import * as commander from "commander";`
};
const cachePath = "/a/cache";
const cachePath = `${projects}/a/cache`;
const commanderJS: TestFSWithWatch.File = {
path: "/node_modules/commander/index.js",
path: `${projects}/node_modules/commander/index.js`,
content: "module.exports = 0",
};

Expand All @@ -1013,10 +1014,17 @@ namespace ts.projectSystem {
const service = createProjectService(host, { typingsInstaller: installer });
service.openClientFile(file.path);

checkWatchedFiles(host, [...flatMap(["/a/b", "/a", ""], x => [x + "/tsconfig.json", x + "/jsconfig.json"]), "/a/lib/lib.d.ts"]);
checkWatchedFiles(host, [...getConfigFilesToWatch(getDirectoryPath(file.path)), "/a/lib/lib.d.ts"]);
checkWatchedDirectories(host, [], /*recursive*/ false);
// Does not include cachePath because that is handled by typingsInstaller
checkWatchedDirectories(host, ["/node_modules", "/a/b/node_modules", "/a/b/node_modules/@types", "/a/b/bower_components"], /*recursive*/ true);
checkWatchedDirectories(host, [
`${projects}/node_modules`,
`${projects}/a/node_modules`,
`${projects}/a/b/node_modules`,
`${projects}/a/node_modules/@types`,
`${projects}/a/b/node_modules/@types`,
`${projects}/a/b/bower_components`
], /*recursive*/ true);

service.checkNumberOfProjects({ inferredProjects: 1 });
checkProjectActualFiles(service.inferredProjects[0], [file.path, commanderJS.path]);
Expand Down