Skip to content

Fixes confusing logs and unnecessary configFile checks from navto by keeping project with result. #39721

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.

Sign up for GitHub

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
Jul 24, 2020
Merged

Fixes confusing logs and unnecessary configFile checks from navto by keeping project with result. #39721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 17 additions & 5 deletions src/server/editorServices.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,6 +629,8 @@ namespace ts.server {
* Open files: with value being project root path, and key being Path of the file that is open
*/
readonly openFiles: Map<NormalizedPath | undefined> = new Map<Path, NormalizedPath | undefined>();
/* @internal */
readonly configFileForOpenFiles: ESMap<Path, NormalizedPath | false> = new Map();
/**
* Map of open files that are opened without complete path but have projectRoot as current directory
*/
Expand Down Expand Up @@ -1461,6 +1463,7 @@ namespace ts.server {
}

this.openFiles.delete(info.path);
this.configFileForOpenFiles.delete(info.path);

if (!skipAssignOrphanScriptInfosToInferredProject && ensureProjectsForOpenFiles) {
this.assignOrphanScriptInfosToInferredProject();
Expand Down Expand Up @@ -1791,7 +1794,11 @@ namespace ts.server {
* otherwise just file name
*/
private getConfigFileNameForFile(info: OpenScriptInfoOrClosedOrConfigFileInfo) {
if (isOpenScriptInfo(info)) Debug.assert(info.isScriptOpen());
if (isOpenScriptInfo(info)) {
Debug.assert(info.isScriptOpen());
const result = this.configFileForOpenFiles.get(info.path);
if (result !== undefined) return result || undefined;
}
this.logger.info(`Search path: ${getDirectoryPath(info.fileName)}`);
const configFileName = this.forEachConfigFileLocation(info, (configFileName, canonicalConfigFilePath) =>
this.configFileExists(configFileName, canonicalConfigFilePath, info));
Expand All @@ -1801,6 +1808,9 @@ namespace ts.server {
else {
this.logger.info(`For info: ${info.fileName} :: No config files found.`);
}
if (isOpenScriptInfo(info)) {
this.configFileForOpenFiles.set(info.path, configFileName || false);
}
return configFileName;
}

Expand Down Expand Up @@ -2171,14 +2181,14 @@ namespace ts.server {
* Read the config file of the project again by clearing the cache and update the project graph
*/
/* @internal */
reloadConfiguredProject(project: ConfiguredProject, reason: string) {
reloadConfiguredProject(project: ConfiguredProject, reason: string, isInitialLoad: boolean) {
// At this point, there is no reason to not have configFile in the host
const host = project.getCachedDirectoryStructureHost();

// Clear the cache since we are reloading the project from disk
host.clearCache();
const configFileName = project.getConfigFilePath();
this.logger.info(`Reloading configured project ${configFileName}`);
this.logger.info(`${isInitialLoad ? "Loading" : "Reloading"} configured project ${configFileName}`);

// Load project from the disk
this.loadConfiguredProject(project, reason);
Expand Down Expand Up @@ -2795,11 +2805,13 @@ namespace ts.server {
const reloadChildProject = (child: ConfiguredProject) => {
if (!updatedProjects.has(child.canonicalConfigFilePath)) {
updatedProjects.set(child.canonicalConfigFilePath, true);
this.reloadConfiguredProject(child, reason);
this.reloadConfiguredProject(child, reason, /*isInitialLoad*/ false);
}
};
// try to reload config file for all open files
openFiles.forEach((openFileValue, path) => {
// Invalidate default config file name for open file
this.configFileForOpenFiles.delete(path);
// Filter out the files that need to be ignored
if (!shouldReloadProjectFor(openFileValue)) {
return;
Expand All @@ -2823,7 +2835,7 @@ namespace ts.server {
}
else {
// reload from the disk
this.reloadConfiguredProject(project, reason);
this.reloadConfiguredProject(project, reason, /*isInitialLoad*/ false);
// If this project does not contain this file directly, reload the project till the reloaded project contains the script info directly
if (!projectContainsInfoDirectly(project, info)) {
const referencedProject = forEachResolvedProjectReferenceProject(
Expand Down
3 changes: 2 additions & 1 deletion src/server/project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2102,6 +2102,7 @@ namespace ts.server {
* @returns: true if set of files in the project stays the same and false - otherwise.
*/
updateGraph(): boolean {
const isInitialLoad = this.isInitialLoadPending();
this.isInitialLoadPending = returnFalse;
const reloadLevel = this.pendingReload;
this.pendingReload = ConfigFileProgramReloadLevel.None;
Expand All @@ -2115,7 +2116,7 @@ namespace ts.server {
this.openFileWatchTriggered.clear();
const reason = Debug.checkDefined(this.pendingReloadReason);
this.pendingReloadReason = undefined;
this.projectService.reloadConfiguredProject(this, reason);
this.projectService.reloadConfiguredProject(this, reason, isInitialLoad);
result = true;
break;
default:
Expand Down
97 changes: 56 additions & 41 deletions src/server/session.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,34 +284,43 @@ namespace ts.server {
return deduplicate(outputs, equateValues);
}

function combineProjectOutputFromEveryProject<T>(projectService: ProjectService, action: (project: Project) => readonly T[], areEqual: (a: T, b: T) => boolean) {
const outputs: T[] = [];
type CombineOutputResult<T> = { project: Project; result: readonly T[]; }[];
function combineOutputResultContains<T>(outputs: CombineOutputResult<T>, output: T, areEqual: (a: T, b: T) => boolean) {
return outputs.some(({ result }) => contains(result, output, areEqual));
}
function addToCombineOutputResult<T>(outputs: CombineOutputResult<T>, project: Project, result: readonly T[]) {
if (result.length) outputs.push({ project, result });
}

function combineProjectOutputFromEveryProject<T>(projectService: ProjectService, action: (project: Project) => readonly T[], areEqual: (a: T, b: T) => boolean): CombineOutputResult<T> {
const outputs: CombineOutputResult<T> = [];
projectService.loadAncestorProjectTree();
projectService.forEachEnabledProject(project => {
const theseOutputs = action(project);
outputs.push(...theseOutputs.filter(output => !outputs.some(o => areEqual(o, output))));
addToCombineOutputResult(outputs, project, filter(theseOutputs, output => !combineOutputResultContains(outputs, output, areEqual)));
});
return outputs;
}

function flattenCombineOutputResult<T>(outputs: CombineOutputResult<T>): readonly T[] {
return flatMap(outputs, ({ result }) => result);
}

function combineProjectOutputWhileOpeningReferencedProjects<T>(
projects: Projects,
defaultProject: Project,
action: (project: Project) => readonly T[],
getLocation: (t: T) => DocumentPosition,
resultsEqual: (a: T, b: T) => boolean,
): T[] {
const outputs: T[] = [];
): CombineOutputResult<T> {
const outputs: CombineOutputResult<T> = [];
combineProjectOutputWorker(
projects,
defaultProject,
/*initialLocation*/ undefined,
(project, _, tryAddToTodo) => {
for (const output of action(project)) {
if (!contains(outputs, output, resultsEqual) && !tryAddToTodo(project, getLocation(output))) {
outputs.push(output);
}
}
const theseOutputs = action(project);
addToCombineOutputResult(outputs, project, filter(theseOutputs, output => !combineOutputResultContains(outputs, output, resultsEqual) && !tryAddToTodo(project, getLocation(output))));
},
);
return outputs;
Expand All @@ -326,7 +335,6 @@ namespace ts.server {
hostPreferences: UserPreferences
): readonly RenameLocation[] {
const outputs: RenameLocation[] = [];

combineProjectOutputWorker(
projects,
defaultProject,
Expand Down Expand Up @@ -1930,38 +1938,42 @@ namespace ts.server {

private getNavigateToItems(args: protocol.NavtoRequestArgs, simplifiedResult: boolean): readonly protocol.NavtoItem[] | readonly NavigateToItem[] {
const full = this.getFullNavigateToItems(args);
return !simplifiedResult ? full : full.map((navItem) => {
const { file, project } = this.getFileAndProject({ file: navItem.fileName });
const scriptInfo = project.getScriptInfo(file)!;
const bakedItem: protocol.NavtoItem = {
name: navItem.name,
kind: navItem.kind,
kindModifiers: navItem.kindModifiers,
isCaseSensitive: navItem.isCaseSensitive,
matchKind: navItem.matchKind,
file: navItem.fileName,
start: scriptInfo.positionToLineOffset(navItem.textSpan.start),
end: scriptInfo.positionToLineOffset(textSpanEnd(navItem.textSpan))
};
if (navItem.kindModifiers && (navItem.kindModifiers !== "")) {
bakedItem.kindModifiers = navItem.kindModifiers;
}
if (navItem.containerName && (navItem.containerName.length > 0)) {
bakedItem.containerName = navItem.containerName;
}
if (navItem.containerKind && (navItem.containerKind.length > 0)) {
bakedItem.containerKind = navItem.containerKind;
}
return bakedItem;
});
return !simplifiedResult ?
flattenCombineOutputResult(full) :
flatMap(
full,
({ project, result }) => result.map(navItem => {
const scriptInfo = project.getScriptInfo(navItem.fileName)!;
const bakedItem: protocol.NavtoItem = {
name: navItem.name,
kind: navItem.kind,
kindModifiers: navItem.kindModifiers,
isCaseSensitive: navItem.isCaseSensitive,
matchKind: navItem.matchKind,
file: navItem.fileName,
start: scriptInfo.positionToLineOffset(navItem.textSpan.start),
end: scriptInfo.positionToLineOffset(textSpanEnd(navItem.textSpan))
};
if (navItem.kindModifiers && (navItem.kindModifiers !== "")) {
bakedItem.kindModifiers = navItem.kindModifiers;
}
if (navItem.containerName && (navItem.containerName.length > 0)) {
bakedItem.containerName = navItem.containerName;
}
if (navItem.containerKind && (navItem.containerKind.length > 0)) {
bakedItem.containerKind = navItem.containerKind;
}
return bakedItem;
})
);
}

private getFullNavigateToItems(args: protocol.NavtoRequestArgs): readonly NavigateToItem[] {
private getFullNavigateToItems(args: protocol.NavtoRequestArgs): CombineOutputResult<NavigateToItem> {
const { currentFileOnly, searchValue, maxResultCount, projectFileName } = args;
if (currentFileOnly) {
Debug.assertDefined(args.file);
const { file, project } = this.getFileAndProject(args as protocol.FileRequestArgs);
return project.getLanguageService().getNavigateToItems(searchValue, maxResultCount, file);
return [{ project, result: project.getLanguageService().getNavigateToItems(searchValue, maxResultCount, file) }];
}
else if (!args.file && !projectFileName) {
return combineProjectOutputFromEveryProject(
Expand Down Expand Up @@ -2082,10 +2094,13 @@ namespace ts.server {
const newPath = toNormalizedPath(args.newFilePath);
const formatOptions = this.getHostFormatOptions();
const preferences = this.getHostPreferences();
const changes = combineProjectOutputFromEveryProject(
this.projectService,
project => project.getLanguageService().getEditsForFileRename(oldPath, newPath, formatOptions, preferences),
(a, b) => a.fileName === b.fileName);
const changes = flattenCombineOutputResult(
combineProjectOutputFromEveryProject(
this.projectService,
project => project.getLanguageService().getEditsForFileRename(oldPath, newPath, formatOptions, preferences),
(a, b) => a.fileName === b.fileName
)
);
return simplifiedResult ? changes.map(c => this.mapTextChangeToCodeEdit(c)) : changes;
}

Expand Down
62 changes: 61 additions & 1 deletion src/testRunner/unittests/tsserver/navTo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,75 @@ namespace ts.projectSystem {
export const ghijkl = a.abcdef;`
};
const host = createServerHost([configFile1, file1, configFile2, file2]);
const session = createSession(host);
const logger = createHasErrorMessageLogger().logger;
const logs: string[] = [];
logger.info = s => logs.push(s);
const session = createSession(host, { logger });
openFilesForSession([file1, file2], session);
logs.length = 0;

const request = makeSessionRequest<protocol.NavtoRequestArgs>(CommandNames.Navto, { searchValue: "abcdef", file: file1.path });
const items = session.executeCommand(request).response as protocol.NavtoItem[];
assert.strictEqual(items.length, 1);
const item = items[0];
assert.strictEqual(item.name, "abcdef");
assert.strictEqual(item.file, file1.path);
assert.deepEqual(logs, []);
});

it("should de-duplicate symbols when searching all projects", () => {
const solutionConfig: File = {
path: "/tsconfig.json",
content: JSON.stringify({
references: [{ path: "./a" }, { path: "./b" }],
files: [],
})
};
const configFile1: File = {
path: "/a/tsconfig.json",
content: `{
"compilerOptions": {
"composite": true
}
}`
};
const file1: File = {
path: "/a/index.ts",
content: "export const abcdef = 1;"
};
const configFile2: File = {
path: "/b/tsconfig.json",
content: `{
"compilerOptions": {
"composite": true
},
"references": [
{ "path": "../a" }
]
}`
};
const file2: File = {
path: "/b/index.ts",
content: `import a = require("../a");
export const ghijkl = a.abcdef;`
};
const host = createServerHost([configFile1, file1, configFile2, file2, solutionConfig]);
const logger = createHasErrorMessageLogger().logger;
const logs: string[] = [];
logger.info = s => logs.push(s);
const session = createSession(host, { logger });
openFilesForSession([file1], session);
logs.length = 0;

const request = makeSessionRequest<protocol.NavtoRequestArgs>(CommandNames.Navto, { searchValue: "abcdef" });
const items = session.executeCommand(request).response as protocol.NavtoItem[];
assert.strictEqual(items.length, 1);
const item = items[0];
assert.strictEqual(item.name, "abcdef");
assert.strictEqual(item.file, file1.path);
// Cannt check logs as empty since it loads projects and writes information for those in the log
assert.isFalse(contains(logs, "Search path: /a"));
assert.isFalse(contains(logs, "For info: /a/index.ts :: Config file name: /a/tsconfig.json"));
});

it("should work with Deprecated", () => {
Expand Down