From 5ca3bcf4f8f6be648ebc4891adec5d45de7ead2d Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 12 Apr 2022 08:56:58 -0700 Subject: [PATCH] fix(language-service): Add resource files as roots to their associated projects (#45601) When an external template is read, adds the template file to to the project which contains. This is necessary to keep the projects open when navigating away from HTML files. Since a `tsconfig` cannot express including non-TS files, we need another way to indicate the template files are considered part of the project. Note that this does not ensure that the project in question _directly_ contains the component file. That is, the project might just include the component file through the program rather than directly in the `include` glob of the `tsconfig`. This distinction is somewhat important because the TypeScript language service/server prefers projects which _directly_ contain the TS file (see `projectContainsInfoDirectly` in the TS codebase). What this means it that there can possibly be a different project used between the TS and HTML files. For example, in Nx projects, the referenced configs are `tsconfig.app.json` and `tsconfig.editor.json`. `tsconfig.app.json` comes first in the base `tsconfig.json` and contains the entry point of the app. `tsconfig.editor.json` contains the `**.ts` glob of all TS files. This means that `tsconfig.editor.json` will be preferred by the TS server for TS files but the `tsconfig.app.json` will be used for HTML files since it comes first and we cannot effectively express `projectContainsInfoDirectly` for HTML files. We could consider also updating the language server implementation to attempt to select the project to use for the template file based on which project contains its component file directly, using either the internal `project.projectContainsInfoDirectly` or as a workaround, check `project.isRoot(componentTsFile)`. Finally, keeping the projects open is hugely important in the solution style config case like Nx. When a TS file is opened, TypeScript will only retain `tsconfig.editor.json` and not `tsconfig.app.json`. However, if our extension does not also know to select `tsconfig.editor.json`, it will automatically select `tsconfig.app.json` since it is defined first in the `tsconfig.json` file. So we need to teach TS server that we are (1) interested in keeping projects open when there is an HTML file open and (2) optionally attempt to do this _only_ for projects that we know the TS language service will prioritize in TS files (i.e., attempt to only keep `tsconfig.editor.json` open and allow `tsconfig.app.json` to close) and prioritize that project for all requests. fixes https://github.com/angular/vscode-ng-language-service/issues/1623 fixes https://github.com/angular/vscode-ng-language-service/issues/876 PR Close #45601 --- packages/compiler-cli/ngcc/BUILD.bazel | 1 + .../ngcc/src/analysis/ngcc_trait_compiler.ts | 3 ++- .../src/ngtsc/core/api/src/adapter.ts | 15 +++++++++++- .../src/ngtsc/core/src/compiler.ts | 2 +- .../compiler-cli/src/ngtsc/core/src/host.ts | 10 ++++++++ .../src/ngtsc/transform/src/compilation.ts | 19 +++++++++------ .../ngtsc/transform/test/compilation_spec.ts | 11 ++++++--- packages/language-service/src/adapters.ts | 24 ++++++++++++++----- .../language-service/testing/src/project.ts | 3 ++- 9 files changed, 68 insertions(+), 20 deletions(-) diff --git a/packages/compiler-cli/ngcc/BUILD.bazel b/packages/compiler-cli/ngcc/BUILD.bazel index 27dad9adecd8a..8aa3a27e8f7ce 100644 --- a/packages/compiler-cli/ngcc/BUILD.bazel +++ b/packages/compiler-cli/ngcc/BUILD.bazel @@ -27,6 +27,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/perf", "//packages/compiler-cli/src/ngtsc/reflection", "//packages/compiler-cli/src/ngtsc/scope", + "//packages/compiler-cli/src/ngtsc/shims", "//packages/compiler-cli/src/ngtsc/sourcemaps", "//packages/compiler-cli/src/ngtsc/transform", "//packages/compiler-cli/src/ngtsc/translator", diff --git a/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts b/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts index 038d52182f472..74a33c8a7a122 100644 --- a/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts +++ b/packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts @@ -11,6 +11,7 @@ import {IncrementalBuild} from '../../../src/ngtsc/incremental/api'; import {SemanticSymbol} from '../../../src/ngtsc/incremental/semantic_graph'; import {NOOP_PERF_RECORDER} from '../../../src/ngtsc/perf'; import {ClassDeclaration, Decorator} from '../../../src/ngtsc/reflection'; +import {isShim} from '../../../src/ngtsc/shims'; import {CompilationMode, DecoratorHandler, DtsTransformRegistry, HandlerFlags, Trait, TraitCompiler} from '../../../src/ngtsc/transform'; import {NgccReflectionHost} from '../host/ngcc_host'; import {isDefined} from '../utils'; @@ -28,7 +29,7 @@ export class NgccTraitCompiler extends TraitCompiler { super( handlers, ngccReflector, NOOP_PERF_RECORDER, new NoIncrementalBuild(), /* compileNonExportedClasses */ true, CompilationMode.FULL, new DtsTransformRegistry(), - /* semanticDepGraphUpdater */ null); + /* semanticDepGraphUpdater */ null, {isShim, isResource: () => false}); } get analyzedFiles(): ts.SourceFile[] { diff --git a/packages/compiler-cli/src/ngtsc/core/api/src/adapter.ts b/packages/compiler-cli/src/ngtsc/core/api/src/adapter.ts index d68c8b99613f6..f3741b8d0efe5 100644 --- a/packages/compiler-cli/src/ngtsc/core/api/src/adapter.ts +++ b/packages/compiler-cli/src/ngtsc/core/api/src/adapter.ts @@ -43,7 +43,8 @@ export interface NgCompilerAdapter extends // incompatible with the `ts.CompilerHost` version which isn't. The combination of these two // still satisfies `ts.ModuleResolutionHost`. Omit, - Pick { + Pick, + SourceFileTypeIdentifier { /** * A path to a single file which represents the entrypoint of an Angular Package Format library, * if the current program is one. @@ -86,7 +87,9 @@ export interface NgCompilerAdapter extends * Resolved list of root directories explicitly set in, or inferred from, the tsconfig. */ readonly rootDirs: ReadonlyArray; +} +export interface SourceFileTypeIdentifier { /** * Distinguishes between shim files added by Angular to the compilation process (both those * intended for output, like ngfactory files, as well as internal shims like ngtypecheck files) @@ -96,4 +99,14 @@ export interface NgCompilerAdapter extends * `true` if a file was written by the user, and `false` if a file was added by the compiler. */ isShim(sf: ts.SourceFile): boolean; + + /** + * Distinguishes between resource files added by Angular to the project and original files in the + * user's program. + * + * This is necessary only for the language service because it adds resource files as root files + * when they are read. This is done to indicate to TS Server that these resources are part of the + * project and ensures that projects are retained properly when navigating around the workspace. + */ + isResource(sf: ts.SourceFile): boolean; } diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index 848203f805696..44a1c965c4e4b 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -1046,7 +1046,7 @@ export class NgCompiler { const traitCompiler = new TraitCompiler( handlers, reflector, this.delegatingPerfRecorder, this.incrementalCompilation, this.options.compileNonExportedClasses !== false, compilationMode, dtsTransforms, - semanticDepGraphUpdater); + semanticDepGraphUpdater, this.adapter); // Template type-checking may use the `ProgramDriver` to produce new `ts.Program`(s). If this // happens, they need to be tracked by the `NgCompiler`. diff --git a/packages/compiler-cli/src/ngtsc/core/src/host.ts b/packages/compiler-cli/src/ngtsc/core/src/host.ts index b3df5e007bbf1..0bd331ac03b8c 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/host.ts @@ -234,6 +234,16 @@ export class NgCompilerHost extends DelegatingCompilerHost implements return isShim(sf); } + /** + * Check whether the given `ts.SourceFile` is a resource file. + * + * This simply returns `false` for the compiler-cli since resource files are not added as root + * files to the project. + */ + isResource(sf: ts.SourceFile): boolean { + return false; + } + getSourceFile( fileName: string, languageVersion: ts.ScriptTarget, onError?: ((message: string) => void)|undefined, diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 6141d3cadb300..8bcd66992777d 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -9,16 +9,16 @@ import {ConstantPool} from '@angular/compiler'; import ts from 'typescript'; +import {SourceFileTypeIdentifier} from '../../core/api'; import {ErrorCode, FatalDiagnosticError} from '../../diagnostics'; import {IncrementalBuild} from '../../incremental/api'; import {SemanticDepGraphUpdater, SemanticSymbol} from '../../incremental/semantic_graph'; import {IndexingContext} from '../../indexer'; import {PerfEvent, PerfRecorder} from '../../perf'; import {ClassDeclaration, DeclarationNode, Decorator, isNamedClassDeclaration, ReflectionHost} from '../../reflection'; -import {isShim} from '../../shims'; import {ProgramTypeCheckAdapter, TypeCheckContext} from '../../typecheck/api'; import {ExtendedTemplateChecker} from '../../typecheck/extended/api'; -import {getSourceFile, isExported} from '../../util/src/typescript'; +import {getSourceFile} from '../../util/src/typescript'; import {Xi18nContext} from '../../xi18n'; import {AnalysisOutput, CompilationMode, CompileResult, DecoratorHandler, HandlerFlags, HandlerPrecedence, ResolveResult} from './api'; @@ -97,11 +97,15 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { constructor( private handlers: DecoratorHandler[], - private reflector: ReflectionHost, private perf: PerfRecorder, + private reflector: ReflectionHost, + private perf: PerfRecorder, private incrementalBuild: IncrementalBuild, - private compileNonExportedClasses: boolean, private compilationMode: CompilationMode, + private compileNonExportedClasses: boolean, + private compilationMode: CompilationMode, private dtsTransforms: DtsTransformRegistry, - private semanticDepGraphUpdater: SemanticDepGraphUpdater|null) { + private semanticDepGraphUpdater: SemanticDepGraphUpdater|null, + private sourceFileTypeIdentifier: SourceFileTypeIdentifier, + ) { for (const handler of handlers) { this.handlersByName.set(handler.name, handler); } @@ -118,8 +122,9 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { private analyze(sf: ts.SourceFile, preanalyze: false): void; private analyze(sf: ts.SourceFile, preanalyze: true): Promise|undefined; private analyze(sf: ts.SourceFile, preanalyze: boolean): Promise|undefined { - // We shouldn't analyze declaration files. - if (sf.isDeclarationFile || isShim(sf)) { + // We shouldn't analyze declaration, shim, or resource files. + if (sf.isDeclarationFile || this.sourceFileTypeIdentifier.isShim(sf) || + this.sourceFileTypeIdentifier.isResource(sf)) { return undefined; } diff --git a/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts b/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts index d0431fe2e3af9..e05f4cf509ad6 100644 --- a/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts +++ b/packages/compiler-cli/src/ngtsc/transform/test/compilation_spec.ts @@ -17,6 +17,11 @@ import {getDeclaration, makeProgram} from '../../testing'; import {CompilationMode, DetectResult, DtsTransformRegistry, TraitCompiler} from '../../transform'; import {AnalysisOutput, CompileResult, DecoratorHandler, HandlerPrecedence} from '../src/api'; +const fakeSfTypeIdentifier = { + isShim: () => false, + isResource: () => false +}; + runInEachFileSystem(() => { describe('TraitCompiler', () => { let _: typeof absoluteFrom; @@ -49,7 +54,7 @@ runInEachFileSystem(() => { const reflectionHost = new TypeScriptReflectionHost(checker); const compiler = new TraitCompiler( [new FakeDecoratorHandler()], reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, - true, CompilationMode.FULL, new DtsTransformRegistry(), null); + true, CompilationMode.FULL, new DtsTransformRegistry(), null, fakeSfTypeIdentifier); const sourceFile = program.getSourceFile('lib.d.ts')!; const analysis = compiler.analyzeSync(sourceFile); @@ -138,7 +143,7 @@ runInEachFileSystem(() => { const compiler = new TraitCompiler( [new PartialDecoratorHandler(), new FullDecoratorHandler()], reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true, CompilationMode.PARTIAL, - new DtsTransformRegistry(), null); + new DtsTransformRegistry(), null, fakeSfTypeIdentifier); const sourceFile = program.getSourceFile('test.ts')!; compiler.analyzeSync(sourceFile); compiler.resolve(); @@ -168,7 +173,7 @@ runInEachFileSystem(() => { const compiler = new TraitCompiler( [new PartialDecoratorHandler(), new FullDecoratorHandler()], reflectionHost, NOOP_PERF_RECORDER, NOOP_INCREMENTAL_BUILD, true, CompilationMode.FULL, - new DtsTransformRegistry(), null); + new DtsTransformRegistry(), null, fakeSfTypeIdentifier); const sourceFile = program.getSourceFile('test.ts')!; compiler.analyzeSync(sourceFile); compiler.resolve(); diff --git a/packages/language-service/src/adapters.ts b/packages/language-service/src/adapters.ts index fc40367efa79b..41f64ea89ba0c 100644 --- a/packages/language-service/src/adapters.ts +++ b/packages/language-service/src/adapters.ts @@ -61,6 +61,11 @@ export class LanguageServiceAdapter implements NgCompilerAdapter { return isShim(sf); } + isResource(sf: ts.SourceFile): boolean { + const scriptInfo = this.project.getScriptInfo(sf.fileName); + return scriptInfo?.scriptKind === ts.ScriptKind.Unknown; + } + fileExists(fileName: string): boolean { return this.project.fileExists(fileName); } @@ -100,14 +105,21 @@ export class LanguageServiceAdapter implements NgCompilerAdapter { // getScriptInfo() will not create one if it does not exist. // In this case, we *want* a script info to be created so that we could // keep track of its version. - const snapshot = this.project.getScriptSnapshot(fileName); - if (!snapshot) { - // This would fail if the file does not exist, or readFile() fails for - // whatever reasons. - throw new Error(`Failed to get script snapshot while trying to read ${fileName}`); - } const version = this.project.getScriptVersion(fileName); this.lastReadResourceVersion.set(fileName, version); + const scriptInfo = this.project.getScriptInfo(fileName); + if (!scriptInfo) { + // // This should not happen because it would have failed already at `getScriptVersion`. + throw new Error(`Failed to get script info when trying to read ${fileName}`); + } + // Add external resources as root files to the project since we project language service + // features for them (this is currently only the case for HTML files, but we could investigate + // css file features in the future). This prevents the project from being closed when navigating + // away from a resource file. + if (!this.project.isRoot(scriptInfo)) { + this.project.addRoot(scriptInfo); + } + const snapshot = scriptInfo.getSnapshot(); return snapshot.getText(0, snapshot.getLength()); } diff --git a/packages/language-service/testing/src/project.ts b/packages/language-service/testing/src/project.ts index c4641ce95b56d..bfdaf3cd628f6 100644 --- a/packages/language-service/testing/src/project.ts +++ b/packages/language-service/testing/src/project.ts @@ -150,7 +150,8 @@ export class Project { const ngCompiler = this.ngLS.compilerFactory.getOrCreate(); for (const sf of program.getSourceFiles()) { - if (sf.isDeclarationFile || sf.fileName.endsWith('.ngtypecheck.ts')) { + if (sf.isDeclarationFile || sf.fileName.endsWith('.ngtypecheck.ts') || + !sf.fileName.endsWith('.ts')) { continue; }