Skip to content

Commit a9b8f54

Browse files
JoostKprofanis
JoostK
authored and
profanis
committed
perf(compiler-cli): only generate directive declarations when used (angular#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 angular#38418
1 parent 9b9a5b1 commit a9b8f54

File tree

3 files changed

+105
-37
lines changed

3 files changed

+105
-37
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,10 @@ class TcbDirectiveTypeOp extends TcbOp {
355355
}
356356

357357
get optional() {
358-
return false;
358+
// The statement generated by this operation is only used to declare the directive's type and
359+
// won't report diagnostics by itself, so the operation is marked as optional to avoid
360+
// generating declarations for directives that don't have any inputs/outputs.
361+
return true;
359362
}
360363

361364
execute(): ts.Identifier {
@@ -387,7 +390,9 @@ class TcbDirectiveCtorOp extends TcbOp {
387390
}
388391

389392
get optional() {
390-
return false;
393+
// The statement generated by this operation is only used to infer the directive's type and
394+
// won't report diagnostics by itself, so the operation is marked as optional.
395+
return true;
391396
}
392397

393398
execute(): ts.Identifier {
@@ -452,7 +457,7 @@ class TcbDirectiveInputsOp extends TcbOp {
452457
}
453458

454459
execute(): null {
455-
const dirId = this.scope.resolve(this.node, this.dir);
460+
let dirId: ts.Expression|null = null;
456461

457462
// TODO(joost): report duplicate properties
458463

@@ -502,6 +507,10 @@ class TcbDirectiveInputsOp extends TcbOp {
502507
// (i.e. private/protected/readonly), generate an assignment into a temporary variable
503508
// that has the type of the field. This achieves type-checking but circumvents the access
504509
// modifiers.
510+
if (dirId === null) {
511+
dirId = this.scope.resolve(this.node, this.dir);
512+
}
513+
505514
const id = this.tcb.allocateId();
506515
const dirTypeRef = this.tcb.env.referenceType(this.dir.ref);
507516
if (!ts.isTypeReferenceNode(dirTypeRef)) {
@@ -515,6 +524,10 @@ class TcbDirectiveInputsOp extends TcbOp {
515524
this.scope.addStatement(temp);
516525
target = id;
517526
} else {
527+
if (dirId === null) {
528+
dirId = this.scope.resolve(this.node, this.dir);
529+
}
530+
518531
// To get errors assign directly to the fields on the instance, using property access
519532
// when possible. String literal fields may not be valid JS identifiers so we use
520533
// literal element access instead for those cases.
@@ -718,7 +731,8 @@ class TcbDirectiveOutputsOp extends TcbOp {
718731
}
719732

720733
execute(): null {
721-
const dirId = this.scope.resolve(this.node, this.dir);
734+
let dirId: ts.Expression|null = null;
735+
722736

723737
// `dir.outputs` is an object map of field names on the directive class to event names.
724738
// 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 {
748762
// that has a `subscribe` method that properly carries the `T` into the handler function.
749763
const handler = tcbCreateEventHandler(output, this.tcb, this.scope, EventParamType.Infer);
750764

765+
if (dirId === null) {
766+
dirId = this.scope.resolve(this.node, this.dir);
767+
}
751768
const outputField = ts.createElementAccess(dirId, ts.createStringLiteral(field));
752769
const outputHelper =
753770
ts.createCall(this.tcb.env.declareOutputHelper(), undefined, [outputField]);

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts

Lines changed: 80 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -156,8 +156,7 @@ describe('type check blocks', () => {
156156
isGeneric: true,
157157
}];
158158
const block = tcb(TEMPLATE, DIRECTIVES);
159-
expect(block).toContain(
160-
'var _t1 = Dir.ngTypeCtor({ "color": (null as any), "strong": (null as any), "enabled": (null as any) });');
159+
expect(block).not.toContain('Dir.ngTypeCtor');
161160
expect(block).toContain('"blue"; false; true;');
162161
});
163162

@@ -204,9 +203,9 @@ describe('type check blocks', () => {
204203
];
205204
expect(tcb(TEMPLATE, DIRECTIVES))
206205
.toContain(
207-
'var _t3 = DirA.ngTypeCtor((null!)); ' +
208-
'var _t2 = DirB.ngTypeCtor({ "inputB": (_t3) }); ' +
209-
'var _t1 = DirA.ngTypeCtor({ "inputA": (_t2) });');
206+
'var _t3 = DirB.ngTypeCtor((null!)); ' +
207+
'var _t2 = DirA.ngTypeCtor({ "inputA": (_t3) }); ' +
208+
'var _t1 = DirB.ngTypeCtor({ "inputB": (_t2) });');
210209
});
211210

212211
it('should handle empty bindings', () => {
@@ -247,9 +246,8 @@ describe('type check blocks', () => {
247246
}];
248247
expect(tcb(TEMPLATE, DIRECTIVES))
249248
.toContain(
250-
'var _t1 = Dir.ngTypeCtor({ "fieldA": (((ctx).foo)) }); ' +
251-
'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
252-
'_t2 = (((ctx).foo));');
249+
'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
250+
'_t1 = (((ctx).foo));');
253251
});
254252
});
255253

@@ -264,6 +262,58 @@ describe('type check blocks', () => {
264262
expect(block).toContain('(ctx).handle(_t1);');
265263
});
266264

265+
it('should only generate directive declarations that have bindings or are referenced', () => {
266+
const TEMPLATE = `
267+
<div
268+
hasInput [input]="value"
269+
hasOutput (output)="handle()"
270+
hasReference #ref="ref"
271+
noReference
272+
noBindings>{{ref.a}}</div>
273+
`;
274+
const DIRECTIVES: TestDeclaration[] = [
275+
{
276+
type: 'directive',
277+
name: 'HasInput',
278+
selector: '[hasInput]',
279+
inputs: {input: 'input'},
280+
},
281+
{
282+
type: 'directive',
283+
name: 'HasOutput',
284+
selector: '[hasOutput]',
285+
outputs: {output: 'output'},
286+
},
287+
{
288+
type: 'directive',
289+
name: 'HasReference',
290+
selector: '[hasReference]',
291+
exportAs: ['ref'],
292+
},
293+
{
294+
type: 'directive',
295+
name: 'NoReference',
296+
selector: '[noReference]',
297+
exportAs: ['no-ref'],
298+
},
299+
{
300+
type: 'directive',
301+
name: 'NoBindings',
302+
selector: '[noBindings]',
303+
inputs: {unset: 'unset'},
304+
},
305+
];
306+
const block = tcb(TEMPLATE, DIRECTIVES);
307+
expect(block).toContain('var _t1: HasInput = (null!)');
308+
expect(block).toContain('_t1.input = (((ctx).value));');
309+
expect(block).toContain('var _t2: HasOutput = (null!)');
310+
expect(block).toContain('_t2["output"]');
311+
expect(block).toContain('var _t3: HasReference = (null!)');
312+
expect(block).toContain('(_t3).a');
313+
expect(block).not.toContain('NoBindings');
314+
expect(block).not.toContain('NoReference');
315+
});
316+
267317
it('should generate a forward element reference correctly', () => {
268318
const TEMPLATE = `
269319
{{ i.value }}
@@ -310,7 +360,7 @@ describe('type check blocks', () => {
310360
inputs: {'color': 'color', 'strong': 'strong', 'enabled': 'enabled'},
311361
}];
312362
const block = tcb(TEMPLATE, DIRECTIVES);
313-
expect(block).toContain('var _t1: Dir = (null!);');
363+
expect(block).not.toContain('var _t1: Dir = (null!);');
314364
expect(block).not.toContain('"color"');
315365
expect(block).not.toContain('"strong"');
316366
expect(block).not.toContain('"enabled"');
@@ -357,10 +407,10 @@ describe('type check blocks', () => {
357407
];
358408
expect(tcb(TEMPLATE, DIRECTIVES))
359409
.toContain(
360-
'var _t1: DirA = (null!); ' +
361-
'var _t2: DirB = (null!); ' +
362-
'_t1.inputA = (_t2); ' +
363-
'_t2.inputA = (_t1);');
410+
'var _t1: DirB = (null!); ' +
411+
'var _t2: DirA = (null!); ' +
412+
'_t2.inputA = (_t1); ' +
413+
'_t1.inputA = (_t2);');
364414
});
365415

366416
it('should handle undeclared properties', () => {
@@ -374,10 +424,9 @@ describe('type check blocks', () => {
374424
},
375425
undeclaredInputFields: ['fieldA']
376426
}];
377-
expect(tcb(TEMPLATE, DIRECTIVES))
378-
.toContain(
379-
'var _t1: Dir = (null!); ' +
380-
'(((ctx).foo)); ');
427+
const block = tcb(TEMPLATE, DIRECTIVES);
428+
expect(block).not.toContain('var _t1: Dir = (null!);');
429+
expect(block).toContain('(((ctx).foo)); ');
381430
});
382431

383432
it('should assign restricted properties to temp variables by default', () => {
@@ -448,9 +497,9 @@ describe('type check blocks', () => {
448497
}];
449498
expect(tcb(TEMPLATE, DIRECTIVES))
450499
.toContain(
451-
'var _t1: Dir = (null!); ' +
452-
'var _t2: typeof Dir.ngAcceptInputType_field1 = (null!); ' +
453-
'_t1.field2 = _t2 = (((ctx).foo));');
500+
'var _t1: typeof Dir.ngAcceptInputType_field1 = (null!); ' +
501+
'var _t2: Dir = (null!); ' +
502+
'_t2.field2 = _t1 = (((ctx).foo));');
454503
});
455504

456505
it('should handle a single property bound to multiple fields, where one of them is undeclared',
@@ -483,11 +532,11 @@ describe('type check blocks', () => {
483532
},
484533
coercedInputFields: ['fieldA'],
485534
}];
486-
expect(tcb(TEMPLATE, DIRECTIVES))
487-
.toContain(
488-
'var _t1: Dir = (null!); ' +
489-
'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
490-
'_t2 = (((ctx).foo));');
535+
const block = tcb(TEMPLATE, DIRECTIVES);
536+
expect(block).not.toContain('var _t1: Dir = (null!);');
537+
expect(block).toContain(
538+
'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
539+
'_t1 = (((ctx).foo));');
491540
});
492541

493542
it('should use coercion types if declared, even when backing field is not declared', () => {
@@ -502,11 +551,11 @@ describe('type check blocks', () => {
502551
coercedInputFields: ['fieldA'],
503552
undeclaredInputFields: ['fieldA'],
504553
}];
505-
expect(tcb(TEMPLATE, DIRECTIVES))
506-
.toContain(
507-
'var _t1: Dir = (null!); ' +
508-
'var _t2: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
509-
'_t2 = (((ctx).foo));');
554+
const block = tcb(TEMPLATE, DIRECTIVES);
555+
expect(block).not.toContain('var _t1: Dir = (null!);');
556+
expect(block).toContain(
557+
'var _t1: typeof Dir.ngAcceptInputType_fieldA = (null!); ' +
558+
'_t1 = (((ctx).foo));');
510559
});
511560

512561
it('should handle $any casts', () => {
@@ -721,7 +770,7 @@ describe('type check blocks', () => {
721770
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
722771
// Note that DOM events are still checked, that is controlled by `checkTypeOfDomEvents`
723772
expect(block).toContain(
724-
'_t2.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
773+
'_t1.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
725774
});
726775
});
727776

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_checker_spec.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -276,11 +276,13 @@ runInEachFileSystem(os => {
276276
selector: '[dir]',
277277
file: dirFile,
278278
type: 'directive',
279+
inputs: {'input': 'input'},
280+
isGeneric: true,
279281
}]
280282
},
281283
{
282284
fileName: dirFile,
283-
source: `export class TestDir {}`,
285+
source: `export class TestDir<T> {}`,
284286
templates: {},
285287
}
286288
],
@@ -294,7 +296,7 @@ runInEachFileSystem(os => {
294296
const tcbReal = templateTypeChecker.getTypeCheckBlock(cmp)!;
295297
expect(tcbReal.getSourceFile().text).not.toContain('TestDir');
296298

297-
templateTypeChecker.overrideComponentTemplate(cmp, '<div dir></div>');
299+
templateTypeChecker.overrideComponentTemplate(cmp, '<div dir [input]="value"></div>');
298300

299301
const tcbOverridden = templateTypeChecker.getTypeCheckBlock(cmp);
300302
expect(tcbOverridden).not.toBeNull();

0 commit comments

Comments
 (0)