Skip to content

Commit

Permalink
refactor(compiler-cli): ensure proper TS incremental program re-use w…
Browse files Browse the repository at this point in the history
…/ signal inputs (angular#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 angular#53521
  • Loading branch information
devversion authored and amilamen committed Jan 26, 2024
1 parent ab81e80 commit 50c3ad3
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
27 changes: 26 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/src/tcb_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.
Expand Down Expand Up @@ -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<ts.ClassDeclaration>, reflector: ReflectionHost,
env: ReferenceEmitEnvironment): boolean {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';


Expand Down Expand Up @@ -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') +
Expand Down
114 changes: 113 additions & 1 deletion packages/compiler-cli/test/ngtsc/incremental_typecheck_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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: '<div [dir]="foo"></div>',
})
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<string>();
}
`);
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<T extends SomeNonExportedClass> {
@Input() dir: T|undefined;
}
`);
env.write('cmp.ts', `
import {Component} from '@angular/core';
@Component({
selector: 'test-cmp',
template: '<div [dir]="foo"></div>',
})
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<T extends SomeNonExportedClass> {
dir = input.required<T>();
}
`);
env.driveMain();

expectCompleteReuse(env.getTsProgram());
expectCompleteReuse(env.getReuseTsProgram());
});
});
});
});

0 comments on commit 50c3ad3

Please sign in to comment.