Skip to content
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

Jump to bottom

Syntax operations also need to ensure project is present for the open script infos since update could be pending to make sure open script info has project #50418

Merged
merged 1 commit into from Aug 24, 2022
Merged

Syntax operations also need to ensure project is present for the open script infos since update could be pending to make sure open script info has project #50418

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/server/session.ts
View file
Open in desktop
Expand Up @@ -1868,9 +1868,9 @@ namespace ts.server {

private getFileAndLanguageServiceForSyntacticOperation(args: protocol.FileRequestArgs) {
// Since this is syntactic operation, there should always be project for the file
// we wouldnt have to ensure project but rather throw if we dont get project
// throw if we dont get project
const file = toNormalizedPath(args.file);
const project = this.getProject(args.projectFileName) || this.projectService.tryGetDefaultProjectForFile(file);
const project = this.getProject(args.projectFileName) || this.projectService.ensureDefaultProjectForFile(file);
amcasey marked this conversation as resolved.
Show resolved Hide resolved
if (!project) {
return Errors.ThrowNoProject();
}
Expand Down
52 changes: 38 additions & 14 deletions src/testRunner/unittests/tsserver/projects.ts
View file
Open in desktop
Expand Up @@ -1054,7 +1054,7 @@ namespace ts.projectSystem {
assert.equal(options.outDir, "C:/a/b", "");
});

it("files opened, closed affecting multiple projects", () => {
it("files opened and closed affecting multiple projects", () => {
const file: File = {
path: "/a/b/projects/config/file.ts",
content: `import {a} from "../files/file1"; export let b = a;`
Expand All @@ -1074,7 +1074,7 @@ namespace ts.projectSystem {

const files = [config, file, filesFile1, filesFile2, libFile];
const host = createServerHost(files);
const session = createSession(host);
const session = createSession(host, { logger: createLoggerWithInMemoryLogs() });
// Create configured project
session.executeCommandSeq<protocol.OpenRequest>({
command: protocol.CommandTypes.Open,
Expand All @@ -1083,18 +1083,13 @@ namespace ts.projectSystem {
}
});

const projectService = session.getProjectService();
const configuredProject = projectService.configuredProjects.get(config.path)!;
verifyConfiguredProject();

// open files/file1 = should not create another project
session.executeCommandSeq<protocol.OpenRequest>({
command: protocol.CommandTypes.Open,
arguments: {
file: filesFile1.path
}
});
verifyConfiguredProject();

// Close the file = should still have project
session.executeCommandSeq<protocol.CloseRequest>({
Expand All @@ -1103,7 +1098,6 @@ namespace ts.projectSystem {
file: file.path
}
});
verifyConfiguredProject();

// Open files/file2 - should create inferred project and close configured project
session.executeCommandSeq<protocol.OpenRequest>({
Expand All @@ -1112,8 +1106,6 @@ namespace ts.projectSystem {
file: filesFile2.path
}
});
checkNumberOfProjects(projectService, { inferredProjects: 1 });
checkProjectActualFiles(projectService.inferredProjects[0], [libFile.path, filesFile2.path]);

// Actions on file1 would result in assert
session.executeCommandSeq<protocol.OccurrencesRequest>({
Expand All @@ -1125,10 +1117,7 @@ namespace ts.projectSystem {
}
});

function verifyConfiguredProject() {
checkNumberOfProjects(projectService, { configuredProjects: 1 });
checkProjectActualFiles(configuredProject, [file.path, filesFile1.path, libFile.path, config.path]);
}
baselineTsserverLogs("projects", "files opened and closed affecting multiple projects", session);
});

it("requests are done on file on pendingReload but has svc for previous version", () => {
Expand Down Expand Up @@ -1620,5 +1609,40 @@ namespace ts.projectSystem {

checkNumberOfInferredProjects(projectService, 0);
});

it("file opened is in configured project that will be removed", () => {
const testsConfig: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig.json`,
content: "{}"
};
const testsFile: File = {
path: `${tscWatch.projectRoot}/playground/tests.ts`,
content: `export function foo() {}`
};
Comment on lines +1614 to +1621
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit of a nit - but really, is the idea that that playground contains some sort of implementation file, and playground/tests would contain the test file? In theory, you should only need 2 tsconfigs and 2 .ts files - but you don't need innerSrcFile, right?

src/
├── tsconfig.json
├── foo.ts
└── spec/
    ├── tsconfig.json
    └── foo.ts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replication of repro at #50131 (comment)

The playground/tsconfig.json is tests config i think or atleast the file opened was test file which imported the inner test file
The innerConfig is the implementation configuration but the test file resides next to it and isnt part of the config

Definitely not ideal configuration for sure. But the PR ensures that we dont crash and project is assigned.
We never look past first tsconfig.json we find because otherwise we would risk loading too many projects when opening file.

const innerFile: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig-json/tests/spec.ts`,
content: `export function bar() { }`
};
const innerConfig: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig-json/tsconfig.json`,
content: JSON.stringify({
include: ["./src"]
})
};
const innerSrcFile: File = {
path: `${tscWatch.projectRoot}/playground/tsconfig-json/src/src.ts`,
content: `export function foobar() { }`
};
const host = createServerHost([testsConfig, testsFile, innerFile, innerConfig, innerSrcFile, libFile]);
const session = createSession(host, { logger: createLoggerWithInMemoryLogs() });
openFilesForSession([testsFile], session);
closeFilesForSession([testsFile], session);
openFilesForSession([innerFile], session);
session.executeCommandSeq<protocol.OutliningSpansRequest>({
command: protocol.CommandTypes.GetOutliningSpans,
arguments: { file: innerFile.path }
});
baselineTsserverLogs("projects", "file opened is in configured project that will be removed", session);
});
});
}