Adds support for inferred project isolation by projectRootPath #17602
Conversation
| if (opts.eventHandler !== undefined) { | ||
| opts.canUseEvents = true; | ||
| } | ||
| return new TestSession({ |
There was a problem hiding this comment.
I think this would be easier to read as:
const defaultOptions: server.sessionOptions = { <<...>> };
return new TestSession({ ...defaultOptions, ...opts });
src/server/editorServices.ts
Outdated
| this.compilerOptionsForInferredProjects = convertCompilerOptions(projectCompilerOptions); | ||
| setCompilerOptionsForInferredProjects(projectCompilerOptions: protocol.ExternalProjectCompilerOptions, projectRootPath?: string): void { | ||
| // ignore this settings if we are not creating inferred projects per project root. | ||
| if (projectRootPath && !this.useInferredProjectPerProjectRoot) return; |
There was a problem hiding this comment.
Can we just assert that this method isn't called with a root path unless those are enabled?
| */ | ||
| readonly openFiles: ScriptInfo[] = []; | ||
|
|
||
| private compilerOptionsForInferredProjects: CompilerOptions; |
There was a problem hiding this comment.
Will this property not be used if compilerOptionsForInferredProjectsPerProjectRoot is? Or do we sometimes use both?
There was a problem hiding this comment.
We would sometimes use both. complierOptionsForInferredProjects would be used for any workspace root that does not have its own distinct workspace settings in your *.code-workspace file.
| this.updateProjectGraphs(this.inferredProjects); | ||
|
|
||
| const updatedProjects: Project[] = []; | ||
| for (const project of this.inferredProjects) { |
There was a problem hiding this comment.
It's a bit more complex than what I'd want to put in a filter. Also a filter would introduce an unnecessary closure.
src/server/editorServices.ts
Outdated
| // ignore this settings if we are not creating inferred projects per project root. | ||
| if (projectRootPath && !this.useInferredProjectPerProjectRoot) return; | ||
|
|
||
| const compilerOptionsForInferredProjects = convertCompilerOptions(projectCompilerOptions); |
There was a problem hiding this comment.
This local variable isn't accurately named any more since it might not be assigned to this.compilerOptionsForInferredProjects.
src/server/editorServices.ts
Outdated
|
|
||
| const updatedProjects: Project[] = []; | ||
| for (const project of this.inferredProjects) { | ||
| if (project.projectRootPath === projectRootPath || (project.projectRootPath && !this.compilerOptionsForInferredProjectsPerProjectRoot.has(project.projectRootPath))) { |
There was a problem hiding this comment.
Don't understand the second half of this condition -- if the project has a root path set but it's somehow not in the map, update it anyway even if its root path doesn't match?
There was a problem hiding this comment.
There is a bug in this logic, it should be more like:
projectRootPath ?
project.projectRootPath === projectRootPath :
!project.projectRootPath || !this.compilerOptionsForInferredProjectsPerProjectRoot.has(project.projectRootPath)There was a problem hiding this comment.
The goal is to use the new compiler options:
- Inferred projects without a projectRootPath, if the new options do not apply to a workspace root
- Inferred projects with a projectRootPath, if the new options do not apply to a workspace root and there is no more specific set of options for that project's root path
- Inferred projects with a projectRootPath, if the new options apply to that project root path.
src/server/editorServices.ts
Outdated
| return this.createInferredProject(/*isSingleInferredProject*/ false, projectRootPath); | ||
| } | ||
|
|
||
| // we don't have an explicit root path, so we should try to find an inferred project that best matches the file. |
There was a problem hiding this comment.
Define best -- it looks like this is finding the project containing the file that is the most deeply nested, so given if we're looking for a project for /a/b/c.ts and we have a choice of projects at /a, /a/b, and /a/b/x, we'll choose /a/b.
| return undefined; | ||
| } | ||
|
|
||
| if (this.inferredProjects.length > 0 && this.inferredProjects[0].projectRootPath === undefined) { |
There was a problem hiding this comment.
If the length isn't always 1, we shouldn't call the option useSingleInferredProject, or at least comment it very well.
Should also comment that the reason we only look at entry 0 is that we always add the "single" inferred project with an unshift, so it will always be at the front.
|
@mjbvz Does this meet the needs of VS Code? |
| project.compileOnSaveEnabled = compilerOptionsForInferredProjects.compileOnSave; | ||
| if (projectRootPath ? | ||
| project.projectRootPath === projectRootPath : | ||
| !project.projectRootPath || !this.compilerOptionsForInferredProjectsPerProjectRoot.has(project.projectRootPath)) { |
There was a problem hiding this comment.
Could you include your comment in the code?
This adds early support to create multiple "inferred projects" isolated by a project root path. Currently this requires providing an additional
--useInferredProjectPerProjectRootargument to tsserver. We may choose to fold this functionality into the existing--useSingleInferredProject, but that could constitute a breaking change.This also adds support to define the default configuration for inferred projects under a project root path, though the addition of an optional
projectRootPathproperty onSetCompilerOptionsForInferredProjectsArgs.Fixes #16961