From fb8f4b4d721e98a3b0f8163af6411dbab35f775e Mon Sep 17 00:00:00 2001 From: JoostK Date: Tue, 11 Aug 2020 23:09:05 +0200 Subject: [PATCH] perf(compiler-cli): only generate directive declarations when used (#38418) The template type-checker would always generate a directive declaration even if its type was never used. For example, directives without any input nor output bindings nor exportAs references don't need the directive to be declared, as its type would never be used. This commit makes the `TcbOp`s that are responsible for declaring a directive as optional, such that they are only executed when requested from another operation. PR Close #38418 --- .../ngtsc/typecheck/src/type_check_block.ts | 25 +++- .../typecheck/test/type_check_block_spec.ts | 111 +++++++++++++----- .../ngtsc/typecheck/test/type_checker_spec.ts | 6 +- 3 files changed, 105 insertions(+), 37 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index ab21246052..5a5535057c 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -355,7 +355,10 @@ class TcbDirectiveTypeOp extends TcbOp { } get optional() { - return false; + // The statement generated by this operation is only used to declare the directive's type and + // won't report diagnostics by itself, so the operation is marked as optional to avoid + // generating declarations for directives that don't have any inputs/outputs. + return true; } execute(): ts.Identifier { @@ -387,7 +390,9 @@ class TcbDirectiveCtorOp extends TcbOp { } get optional() { - return false; + // The statement generated by this operation is only used to infer the directive's type and + // won't report diagnostics by itself, so the operation is marked as optional. + return true; } execute(): ts.Identifier { @@ -452,7 +457,7 @@ class TcbDirectiveInputsOp extends TcbOp { } execute(): null { - const dirId = this.scope.resolve(this.node, this.dir); + let dirId: ts.Expression|null = null; // TODO(joost): report duplicate properties @@ -502,6 +507,10 @@ class TcbDirectiveInputsOp extends TcbOp { // (i.e. private/protected/readonly), generate an assignment into a temporary variable // that has the type of the field. This achieves type-checking but circumvents the access // modifiers. + if (dirId === null) { + dirId = this.scope.resolve(this.node, this.dir); + } + const id = this.tcb.allocateId(); const dirTypeRef = this.tcb.env.referenceType(this.dir.ref); if (!ts.isTypeReferenceNode(dirTypeRef)) { @@ -515,6 +524,10 @@ class TcbDirectiveInputsOp extends TcbOp { this.scope.addStatement(temp); target = id; } else { + if (dirId === null) { + dirId = this.scope.resolve(this.node, this.dir); + } + // To get errors assign directly to the fields on the instance, using property access // when possible. String literal fields may not be valid JS identifiers so we use // literal element access instead for those cases. @@ -718,7 +731,8 @@ class TcbDirectiveOutputsOp extends TcbOp { } execute(): null { - const dirId = this.scope.resolve(this.node, this.dir); + let dirId: ts.Expression|null = null; + // `dir.outputs` is an object map of field names on the directive class to event names. // This is backwards from what's needed to match event handlers - a map of event names to field @@ -748,6 +762,9 @@ class TcbDirectiveOutputsOp extends TcbOp { // that has a `subscribe` method that properly carries the `T` into the handler function. const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Infer); + if (dirId === null) { + dirId = this.scope.resolve(this.node, this.dir); + } const outputField = ts.createElementAccess(dirId, ts.createStringLiteral(field)); const outputHelper = ts.createCall(this.tcb.env.declareOutputHelper(), undefined, [outputField]); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index 27cb1ac6e3..ab7c6e73aa 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -156,8 +156,7 @@ describe('type check blocks', () => { isGeneric: true, }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain( - 'var _t1 = Dir.ngTypeCtor({ "color": (null as any), "strong": (null as any), "enabled": (null as any) });'); + expect(block).not.toContain('Dir.ngTypeCtor'); expect(block).toContain('"blue"; false; true;'); }); @@ -204,9 +203,9 @@ describe('type check blocks', () => { ]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t3 = DirA.ngTypeCtor((null!)); ' + - 'var _t2 = DirB.ngTypeCtor({ "inputB": (_t3) }); ' + - 'var _t1 = DirA.ngTypeCtor({ "inputA": (_t2) });'); + 'var _t3 = DirB.ngTypeCtor((null!)); ' + + 'var _t2 = DirA.ngTypeCtor({ "inputA": (_t3) }); ' + + 'var _t1 = DirB.ngTypeCtor({ "inputB": (_t2) });'); }); it('should handle empty bindings', () => { @@ -247,9 +246,8 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1 = Dir.ngTypeCtor({ "fieldA": (((ctx).foo)) }); ' + - 'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + - '_t2 = (((ctx).foo));'); + 'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t1 = (((ctx).foo));'); }); }); @@ -264,6 +262,58 @@ describe('type check blocks', () => { expect(block).toContain('(ctx).handle(_t1);'); }); + it('should only generate directive declarations that have bindings or are referenced', () => { + const TEMPLATE = ` +
{{ref.a}}
+ `; + const DIRECTIVES: TestDeclaration[] = [ + { + type: 'directive', + name: 'HasInput', + selector: '[hasInput]', + inputs: {input: 'input'}, + }, + { + type: 'directive', + name: 'HasOutput', + selector: '[hasOutput]', + outputs: {output: 'output'}, + }, + { + type: 'directive', + name: 'HasReference', + selector: '[hasReference]', + exportAs: ['ref'], + }, + { + type: 'directive', + name: 'NoReference', + selector: '[noReference]', + exportAs: ['no-ref'], + }, + { + type: 'directive', + name: 'NoBindings', + selector: '[noBindings]', + inputs: {unset: 'unset'}, + }, + ]; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('var _t1: HasInput = (null!)'); + expect(block).toContain('_t1.input = (((ctx).value));'); + expect(block).toContain('var _t2: HasOutput = (null!)'); + expect(block).toContain('_t2["output"]'); + expect(block).toContain('var _t3: HasReference = (null!)'); + expect(block).toContain('(_t3).a'); + expect(block).not.toContain('NoBindings'); + expect(block).not.toContain('NoReference'); + }); + it('should generate a forward element reference correctly', () => { const TEMPLATE = ` {{ i.value }} @@ -310,7 +360,7 @@ describe('type check blocks', () => { inputs: {'color': 'color', 'strong': 'strong', 'enabled': 'enabled'}, }]; const block = tcb(TEMPLATE, DIRECTIVES); - expect(block).toContain('var _t1: Dir = (null!);'); + expect(block).not.toContain('var _t1: Dir = (null!);'); expect(block).not.toContain('"color"'); expect(block).not.toContain('"strong"'); expect(block).not.toContain('"enabled"'); @@ -357,10 +407,10 @@ describe('type check blocks', () => { ]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: DirA = (null!); ' + - 'var _t2: DirB = (null!); ' + - '_t1.inputA = (_t2); ' + - '_t2.inputA = (_t1);'); + 'var _t1: DirB = (null!); ' + + 'var _t2: DirA = (null!); ' + + '_t2.inputA = (_t1); ' + + '_t1.inputA = (_t2);'); }); it('should handle undeclared properties', () => { @@ -374,10 +424,9 @@ describe('type check blocks', () => { }, undeclaredInputFields: ['fieldA'] }]; - expect(tcb(TEMPLATE, DIRECTIVES)) - .toContain( - 'var _t1: Dir = (null!); ' + - '(((ctx).foo)); '); + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).not.toContain('var _t1: Dir = (null!);'); + expect(block).toContain('(((ctx).foo)); '); }); it('should assign restricted properties to temp variables by default', () => { @@ -448,9 +497,9 @@ describe('type check blocks', () => { }]; expect(tcb(TEMPLATE, DIRECTIVES)) .toContain( - 'var _t1: Dir = (null!); ' + - 'var _t2: typeof Dir.ngAcceptInputType_field1 = (null!); ' + - '_t1.field2 = _t2 = (((ctx).foo));'); + 'var _t1: typeof Dir.ngAcceptInputType_field1 = (null!); ' + + 'var _t2: Dir = (null!); ' + + '_t2.field2 = _t1 = (((ctx).foo));'); }); it('should handle a single property bound to multiple fields, where one of them is undeclared', @@ -483,11 +532,11 @@ describe('type check blocks', () => { }, coercedInputFields: ['fieldA'], }]; - expect(tcb(TEMPLATE, DIRECTIVES)) - .toContain( - 'var _t1: Dir = (null!); ' + - 'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + - '_t2 = (((ctx).foo));'); + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).not.toContain('var _t1: Dir = (null!);'); + expect(block).toContain( + 'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t1 = (((ctx).foo));'); }); it('should use coercion types if declared, even when backing field is not declared', () => { @@ -502,11 +551,11 @@ describe('type check blocks', () => { coercedInputFields: ['fieldA'], undeclaredInputFields: ['fieldA'], }]; - expect(tcb(TEMPLATE, DIRECTIVES)) - .toContain( - 'var _t1: Dir = (null!); ' + - 'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + - '_t2 = (((ctx).foo));'); + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).not.toContain('var _t1: Dir = (null!);'); + expect(block).toContain( + 'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' + + '_t1 = (((ctx).foo));'); }); it('should handle $any casts', () => { @@ -721,7 +770,7 @@ describe('type check blocks', () => { expect(block).toContain('function ($event: any): any { (ctx).foo($event); }'); // Note that DOM events are still checked, that is controlled by `checkTypeOfDomEvents` expect(block).toContain( - '_t2.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });'); + '_t1.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });'); }); }); diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts index e2a14bce68..c6e148b961 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts @@ -276,11 +276,13 @@ runInEachFileSystem(os => { selector: '[dir]', file: dirFile, type: 'directive', + inputs: {'input': 'input'}, + isGeneric: true, }] }, { fileName: dirFile, - source: `export class TestDir {}`, + source: `export class TestDir {}`, templates: {}, } ], @@ -294,7 +296,7 @@ runInEachFileSystem(os => { const tcbReal = templateTypeChecker.getTypeCheckBlock(cmp)!; expect(tcbReal.getSourceFile().text).not.toContain('TestDir'); - templateTypeChecker.overrideComponentTemplate(cmp, '
'); + templateTypeChecker.overrideComponentTemplate(cmp, '
'); const tcbOverridden = templateTypeChecker.getTypeCheckBlock(cmp); expect(tcbOverridden).not.toBeNull();