From e6db288ffd30b9807dee4dc4cdd6e4615ffca4d1 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Wed, 13 Dec 2023 21:41:32 +0000 Subject: [PATCH] refactor(compiler-cli): ensure proper TS incremental program re-use w/ signal inputs (#53521) Whenever a signal input is captured in a type check block, we will insert an import. This will change the import graph so that the full TypeScript program cannot be structurally re-used. We can fix this trivially by ensuring the import graph remains stable, by always generating an import to e.g. `@angular/core`. This fixes the issue nicely for type-check block files. A test verifies this. For inline code, such as TCB inline or the type constructors inline, this fix is not applicable because we would change user-input source files, adding new edges that would not exist for subsequent builds- causing the program to be not re-used completely. One idea was to rely on the existing edge that can be assumed to exist for directive code files. This is true technically, but in practice TS does not deduplicate imports- so our new namespace import when referencing our symbols will invalidate the re-use. We will address this in a follow-up. There are a couple of options, such as working with the TS team, updating the existing edge, or inlining our helpers as well. PR Close #53521 --- .../src/reference_emit_environment.ts | 2 +- .../src/ngtsc/typecheck/src/tcb_util.ts | 27 ++++- .../ngtsc/typecheck/src/type_check_file.ts | 7 ++ .../test/ngtsc/incremental_typecheck_spec.ts | 114 +++++++++++++++++- 4 files changed, 147 insertions(+), 3 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts index 2e5054a2667..ea336d57bb7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/reference_emit_environment.ts @@ -21,7 +21,7 @@ import {ImportManager, translateExpression, translateType} from '../../translato */ export class ReferenceEmitEnvironment { constructor( - protected importManager: ImportManager, protected refEmitter: ReferenceEmitter, + readonly importManager: ImportManager, protected refEmitter: ReferenceEmitter, readonly reflector: ReflectionHost, protected contextFile: ts.SourceFile) {} canReferenceType( diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts index 87a21bfd72a..bc396d7f9d2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {AbsoluteSourceSpan, ParseSourceSpan} from '@angular/compiler'; +import {AbsoluteSourceSpan, ParseSourceSpan, R3Identifiers} from '@angular/compiler'; import ts from 'typescript'; import {ClassDeclaration, ReflectionHost} from '../../../../src/ngtsc/reflection'; @@ -18,6 +18,20 @@ import {hasIgnoreForDiagnosticsMarker, readSpanComment} from './comments'; import {ReferenceEmitEnvironment} from './reference_emit_environment'; import {TypeParameterEmitter} from './type_parameter_emitter'; +/** + * External modules that always should exist for type check blocks and + * file hosting inline type constructors. + * + * Importing the modules in preparation helps ensuring a stable import graph + * that would not degrade TypeScript's incremental program structure re-use. + */ +const TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES = [ + // Imports may be added for signal input checking. We wouldn't want to change the + // import graph for incremental compilations when suddenly a signal input is used, + // or removed. + R3Identifiers.InputSignalBrandWriteType.moduleName, +]; + /** * Adapter interface which allows the template type-checking diagnostics code to interpret offsets * in a TCB and map them back to original locations in the template. @@ -174,6 +188,17 @@ function getTemplateId( }) as TemplateId || null; } +/** + * Ensure imports for certain external modules that should always + * exist are generated. These are ensures to exist to avoid frequent + * import graph changes whenever e.g. a signal input is introduced in user code. + */ +export function ensureTypeCheckFilePreparationImports(env: ReferenceEmitEnvironment): void { + for (const moduleName of TCB_FILE_IMPORT_GRAPH_PREPARE_MODULES) { + env.importManager.generateNamespaceImport(moduleName); + } +} + export function checkIfGenericTypeBoundsCanBeEmitted( node: ClassDeclaration, reflector: ReflectionHost, env: ReferenceEmitEnvironment): boolean { diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts index 4a14ff43da6..eb9c71d7466 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts @@ -16,6 +16,7 @@ import {TypeCheckBlockMetadata, TypeCheckingConfig} from '../api'; import {DomSchemaChecker} from './dom'; import {Environment} from './environment'; import {OutOfBandDiagnosticRecorder} from './oob'; +import {ensureTypeCheckFilePreparationImports} from './tcb_util'; import {generateTypeCheckBlock, TcbGenericContextBehavior} from './type_check_block'; @@ -52,6 +53,12 @@ export class TypeCheckFile extends Environment { } render(removeComments: boolean): string { + // NOTE: We are conditionally adding imports whenever we discover signal inputs. This has a + // risk of changing the import graph of the TypeScript program, degrading incremental program + // re-use due to program structure changes. For type check block files, we are ensuring an + // import to e.g. `@angular/core` always exists to guarantee a stable graph. + ensureTypeCheckFilePreparationImports(this); + let source: string = this.importManager.getAllImports(this.contextFile.fileName) .map(i => `import * as ${i.qualifier.text} from '${i.specifier}';`) .join('\n') + diff --git a/packages/compiler-cli/test/ngtsc/incremental_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_typecheck_spec.ts index 72d0b1b1efb..439046b60d6 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_typecheck_spec.ts @@ -10,7 +10,7 @@ import ts from 'typescript'; import {absoluteFrom} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; -import {loadStandardTestFiles} from '../../src/ngtsc/testing'; +import {expectCompleteReuse, loadStandardTestFiles} from '../../src/ngtsc/testing'; import {NgtscTestEnvironment} from './env'; @@ -1392,5 +1392,117 @@ runInEachFileSystem(() => { `Can't bind to 'grandgrandparentA' since it isn't a known property of 'div'.`); }); }); + + describe('program re-use', () => { + it('should completely re-use structure when the first signal input is introduced', () => { + env.tsconfig({strictTemplates: true}); + env.write('dir.ts', ` + import {Directive, Input} from '@angular/core'; + + @Directive({ + selector: '[dir]', + }) + export class Dir { + @Input() dir: string = ''; + } + `); + env.write('cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '
', + }) + export class Cmp { + foo = 'foo'; + } + `); + env.write('mod.ts', ` + import {NgModule} from '@angular/core'; + import {Cmp} from './cmp'; + import {Dir} from './dir'; + + @NgModule({ + declarations: [Cmp, Dir], + }) + export class Mod {} + `); + env.driveMain(); + + // introduce the signal input. + env.write('dir.ts', ` + import {Directive, input} from '@angular/core'; + + @Directive({ + selector: '[dir]', + }) + export class Dir { + dir = input.required(); + } + `); + env.driveMain(); + + expectCompleteReuse(env.getTsProgram()); + expectCompleteReuse(env.getReuseTsProgram()); + }); + + // TODO(devversion): look into fixing this for inline TCB and inline type ctors. + xit('should completely re-use structure when an inline constructor generic directive starts using input signals', + () => { + env.tsconfig({strictTemplates: true}); + env.write('dir.ts', ` + import {Directive, Input} from '@angular/core'; + + class SomeNonExportedClass {} + + @Directive({ + selector: '[dir]', + }) + export class Dir { + @Input() dir: T|undefined; + } + `); + env.write('cmp.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: '
', + }) + export class Cmp { + foo = 'foo'; + } + `); + env.write('mod.ts', ` + import {NgModule} from '@angular/core'; + import {Cmp} from './cmp'; + import {Dir} from './dir'; + + @NgModule({ + declarations: [Cmp, Dir], + }) + export class Mod {} + `); + env.driveMain(); + + // turn the input into a signal input- causing a new import. + env.write('dir.ts', ` + import {Directive, input} from '@angular/core'; + + class SomeNonExportedClass {} + + @Directive({ + selector: '[dir]', + }) + export class Dir { + dir = input.required(); + } + `); + env.driveMain(); + + expectCompleteReuse(env.getTsProgram()); + expectCompleteReuse(env.getReuseTsProgram()); + }); + }); }); });