From 12fd06916bd78f8e282af960db40f07fea221e6b Mon Sep 17 00:00:00 2001 From: crisbeto Date: Sat, 13 Jul 2019 13:34:34 +0200 Subject: [PATCH] fix(ivy): don't match directives against attribute bindings (#31541) Fixes Ivy matching directives against attribute bindings (e.g. `[attr.some-directive]="foo"`). Works by excluding attribute bindings from the attributes array during compilation. This has the added benefit of generating less code. **Note:** My initial approach to implementing this was to have a different marker for attribute bindings so that they can be ignored when matching directives, however as I was implementing it I realized that the attributes in that array were only used for directive matching (as far as I could tell). I decided to drop the attribute bindings completely, because it results in less generated code. PR Close #31541 --- .../r3_view_compiler_binding_spec.ts | 35 +++++++++++++++++++ .../compliance/r3_view_compiler_i18n_spec.ts | 2 +- .../r3_view_compiler_styling_spec.ts | 6 ++-- .../compiler/src/render3/view/template.ts | 10 +++--- packages/core/src/render3/interfaces/node.ts | 2 +- .../core/test/acceptance/directive_spec.ts | 24 +++++++++++++ 6 files changed, 70 insertions(+), 9 deletions(-) diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts index e84584964b..fc89d8e9f6 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_binding_spec.ts @@ -612,6 +612,41 @@ describe('compiler compliance: bindings', () => { expectEmit(result.source, template, 'Incorrect template'); }); + it('should exclude attribute bindings from the attributes array', () => { + const files: MockDirectory = { + app: { + 'example.ts': ` + import {Component, NgModule} from '@angular/core'; + + @Component({ + selector: 'my-app', + template: \`\` + }) + export class MyComponent { + doThings() {} + } + + @NgModule({declarations: [MyComponent]}) + export class MyModule {}` + } + }; + + const template = ` + const $e0_attrs$ = ["target", "_blank", "aria-label", "link", ${AttributeMarker.Bindings}, "title", "id", "customEvent"]; + … + `; + const result = compile(files, angularFiles); + expectEmit(result.source, template, 'Incorrect attribute array'); + }); + }); describe('host bindings', () => { diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts index 5780326502..8465b86b40 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_i18n_spec.ts @@ -333,7 +333,7 @@ describe('i18n support in the view compiler', () => { `; const output = ` - const $_c0$ = [${AttributeMarker.Bindings}, "title", "label"]; + const $_c0$ = [3, "title"]; … template: function MyComponent_Template(rf, ctx) { if (rf & 1) { diff --git a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts index bacf64a1c4..478e3b9811 100644 --- a/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts +++ b/packages/compiler-cli/test/compliance/r3_view_compiler_styling_spec.ts @@ -520,7 +520,7 @@ describe('compiler compliance: styling', () => { }; const template = ` - const $_c0$ = [${AttributeMarker.Styles}, "opacity", "1", ${AttributeMarker.Bindings}, "style"]; + const $_c0$ = [${AttributeMarker.Styles}, "opacity", "1"]; const $_c1$ = ["width", "height"]; … MyComponent.ngComponentDef = $r3$.ɵɵdefineComponent({ @@ -741,7 +741,7 @@ describe('compiler compliance: styling', () => { }; const template = ` - const $e0_attrs$ = [${AttributeMarker.Classes}, "grape", ${AttributeMarker.Bindings}, "class"]; + const $e0_attrs$ = [${AttributeMarker.Classes}, "grape"]; const $e0_bindings$ = ["apple", "orange"]; … MyComponent.ngComponentDef = $r3$.ɵɵdefineComponent({ @@ -797,7 +797,7 @@ describe('compiler compliance: styling', () => { }; const template = ` - const $e0_attrs$ = [${AttributeMarker.Classes}, "foo", ${AttributeMarker.Styles}, "width", "100px", ${AttributeMarker.Bindings}, "class", "style"]; + const $e0_attrs$ = [${AttributeMarker.Classes}, "foo", ${AttributeMarker.Styles}, "width", "100px"]; … MyComponent.ngComponentDef = $r3$.ɵɵdefineComponent({ type: MyComponent, diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index a384a22622..12dec412a2 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -1253,11 +1253,13 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } if (inputs.length || outputs.length) { - const attrsStartIndex = attrExprs.length; + const attrsLengthBeforeInputs = attrExprs.length; for (let i = 0; i < inputs.length; i++) { const input = inputs[i]; - if (input.type !== BindingType.Animation) { + // We don't want the animation and attribute bindings in the + // attributes array since they aren't used for directive matching. + if (input.type !== BindingType.Animation && input.type !== BindingType.Attribute) { addAttrExpr(input.name); } } @@ -1273,8 +1275,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // values have been filtered (by not including the animation ones) and added // to the expressions. The marker is important because it tells the runtime // code that this is where attributes without values start... - if (attrExprs.length) { - attrExprs.splice(attrsStartIndex, 0, o.literal(core.AttributeMarker.Bindings)); + if (attrExprs.length !== attrsLengthBeforeInputs) { + attrExprs.splice(attrsLengthBeforeInputs, 0, o.literal(core.AttributeMarker.Bindings)); } } diff --git a/packages/core/src/render3/interfaces/node.ts b/packages/core/src/render3/interfaces/node.ts index 6f10e9134b..9ceb418289 100644 --- a/packages/core/src/render3/interfaces/node.ts +++ b/packages/core/src/render3/interfaces/node.ts @@ -199,7 +199,7 @@ export const enum AttributeMarker { * ``` * var _c1 = ['moo', 'car', AttributeMarker.I18n, 'foo', 'bar']; */ - I18n, + I18n = 6, } /** diff --git a/packages/core/test/acceptance/directive_spec.ts b/packages/core/test/acceptance/directive_spec.ts index e6d7dde5c7..ee411dcd4c 100644 --- a/packages/core/test/acceptance/directive_spec.ts +++ b/packages/core/test/acceptance/directive_spec.ts @@ -202,6 +202,30 @@ describe('directives', () => { expect(fixture.debugElement.query(By.directive(TestDir))).toBeTruthy(); }); + it('should not match directives based on attribute bindings', () => { + const calls: string[] = []; + + @Directive({selector: '[dir]'}) + class MyDir { + ngOnInit() { calls.push('MyDir.ngOnInit'); } + } + + @Component({ + selector: `my-comp`, + template: `

`, + }) + class MyComp { + direction = 'auto'; + } + + TestBed.configureTestingModule({declarations: [MyDir, MyComp]}); + const fixture = TestBed.createComponent(MyComp); + fixture.detectChanges(); + + // Expect only one directive to be instantiated. + expect(calls).toEqual(['MyDir.ngOnInit']); + }); + }); describe('outputs', () => {