From 23c017121e6f8606ca7b3580a64f7718d5b4d91d Mon Sep 17 00:00:00 2001 From: crisbeto Date: Mon, 24 Jun 2019 20:24:32 +0200 Subject: [PATCH] perf(ivy): chain multiple attribute instructions (#31152) Similarly to what we did in #31078, these changes implement chaining for the `attribute` instruction. This PR resolves FW-1390. PR Close #31152 --- .../r3_view_compiler_binding_spec.ts | 208 +++++++++++++++++- .../r3_view_compiler_styling_spec.ts | 3 +- .../compiler/src/render3/view/template.ts | 44 ++-- .../src/render3/instructions/attribute.ts | 8 +- .../core/test/acceptance/attributes_spec.ts | 98 +++++++++ tools/public_api_guard/core/core.d.ts | 2 +- 6 files changed, 335 insertions(+), 28 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 b522b764cb..f86841dc00 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 @@ -229,9 +229,9 @@ describe('compiler compliance: bindings', () => { template: function MyComponent_Template(rf, ctx) { … if (rf & 2) { - $r3$.ɵɵattribute("id", 2); $r3$.ɵɵpropertyInterpolate("aria-label", 1 + 3); $r3$.ɵɵproperty("title", 1)("tabindex", 3); + $r3$.ɵɵattribute("id", 2); } } `; @@ -408,6 +408,212 @@ describe('compiler compliance: bindings', () => { }); + describe('attribute bindings', () => { + it('should chain multiple attribute bindings into a single instruction', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + + \` + }) + export class MyComponent { + myTitle = 'hello'; + buttonId = 'special-button'; + }` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattribute("title", ctx.myTitle)("id", ctx.buttonId)("tabindex", 1); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + it('should chain multiple single-interpolation attribute bindings into one instruction', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + + \` + }) + export class MyComponent { + myTitle = 'hello'; + buttonId = 'special-button'; + }` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattribute("title", ctx.myTitle)("id", ctx.buttonId)("tabindex", 1); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + it('should chain attribute bindings in the presence of other instructions', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + + \` + }) + export class MyComponent {}` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattributeInterpolate1("aria-label", "prefix-", 1 + 3, ""); + $r3$.ɵɵproperty("id", 2); + $r3$.ɵɵattribute("title", 1)("tabindex", 3); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + it('should not add interpolated attributes to the attribute instruction chain', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + \` + }) + export class MyComponent {}` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattributeInterpolate1("tabindex", "prefix-", 0 + 3, ""); + $r3$.ɵɵattributeInterpolate2("aria-label", "hello-", 1 + 3, "-", 2 + 3, ""); + $r3$.ɵɵattribute("title", 1)("id", 2); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + it('should chain multiple attribute bindings when there are multiple elements', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + + + + \` + }) + export class MyComponent { + myTitle = 'hello'; + buttonId = 'special-button'; + }` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattribute("title", ctx.myTitle)("id", ctx.buttonId)("tabindex", 1); + $r3$.ɵɵselect(1); + $r3$.ɵɵattribute("id", 1)("title", "hello")("some-attr", 1 + 2); + $r3$.ɵɵselect(2); + $r3$.ɵɵattribute("some-attr", "one")("some-other-attr", 2); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + it('should chain multiple attribute bindings when there are child elements', () => { + const files = { + app: { + 'example.ts': ` + import {Component} from '@angular/core'; + + @Component({ + template: \` + \` + }) + export class MyComponent { + myTitle = 'hello'; + buttonId = 'special-button'; + }` + } + }; + + const result = compile(files, angularFiles); + const template = ` + … + template: function MyComponent_Template(rf, ctx) { + … + if (rf & 2) { + $r3$.ɵɵattribute("title", ctx.myTitle)("id", ctx.buttonId)("tabindex", 1); + $r3$.ɵɵselect(1); + $r3$.ɵɵattribute("id", 1)("title", "hello")("some-attr", 1 + 2); + } + } + `; + + expectEmit(result.source, template, 'Incorrect template'); + }); + + }); + describe('host bindings', () => { it('should support host bindings', () => { const files = { 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 8fa503900f..ab4058c47a 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 @@ -812,8 +812,7 @@ describe('compiler compliance: styling', () => { $r3$.ɵɵelement(0, "div", $e0_attrs$); } if (rf & 2) { - $r3$.ɵɵattribute("class", "round"); - $r3$.ɵɵattribute("style", "height:100px", $r3$.ɵɵsanitizeStyle); + $r3$.ɵɵattribute("class", "round")("style", "height:100px", $r3$.ɵɵsanitizeStyle); } }, encapsulation: 2 diff --git a/packages/compiler/src/render3/view/template.ts b/packages/compiler/src/render3/view/template.ts index 17a6d92810..e93726cd91 100644 --- a/packages/compiler/src/render3/view/template.ts +++ b/packages/compiler/src/render3/view/template.ts @@ -711,7 +711,8 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver // special value to symbolize that there is no RHS to this binding // TODO (matsko): revisit this once FW-959 is approached const emptyValueBindInstruction = o.literal(undefined); - const propertyBindings: ChainablePropertyBinding[] = []; + const propertyBindings: ChainableBindingInstruction[] = []; + const attributeBindings: ChainableBindingInstruction[] = []; // Generate element input bindings allOtherInputs.forEach((input: t.BoundAttribute) => { @@ -781,8 +782,12 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } else { const boundValue = value instanceof Interpolation ? value.expressions[0] : value; // [attr.name]="value" or attr.name="{{value}}" - this.boundUpdateInstruction( - R3.attribute, elementIndex, attrName, input, boundValue, params); + // Collect the attribute bindings so that they can be chained at the end. + attributeBindings.push({ + name: attrName, + input, + value: () => this.convertPropertyBinding(boundValue), params + }); } } else { // class prop @@ -798,7 +803,11 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver }); if (propertyBindings.length > 0) { - this.propertyInstructionChain(elementIndex, propertyBindings); + this.updateInstructionChain(elementIndex, R3.property, propertyBindings); + } + + if (attributeBindings.length > 0) { + this.updateInstructionChain(elementIndex, R3.attribute, attributeBindings); } // Traverse element child nodes @@ -1025,7 +1034,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver private templatePropertyBindings( templateIndex: number, attrs: (t.BoundAttribute|t.TextAttribute)[]) { - const propertyBindings: ChainablePropertyBinding[] = []; + const propertyBindings: ChainableBindingInstruction[] = []; attrs.forEach(input => { if (input instanceof t.BoundAttribute) { const value = input.value.visit(this._valueConverter); @@ -1039,7 +1048,7 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver }); if (propertyBindings.length > 0) { - this.propertyInstructionChain(templateIndex, propertyBindings); + this.updateInstructionChain(templateIndex, R3.property, propertyBindings); } } @@ -1083,23 +1092,16 @@ export class TemplateDefinitionBuilder implements t.Visitor, LocalResolver } private updateInstructionChain( - nodeIndex: number, span: ParseSourceSpan|null, reference: o.ExternalReference, - callsOrFn?: o.Expression[][]|(() => o.Expression[][])) { + nodeIndex: number, reference: o.ExternalReference, bindings: ChainableBindingInstruction[]) { + const span = bindings.length ? bindings[0].input.sourceSpan : null; + this.addSelectInstructionIfNecessary(nodeIndex, span); this._updateCodeFns.push(() => { - const calls = typeof callsOrFn === 'function' ? callsOrFn() : callsOrFn; - return chainedInstruction(span, reference, calls || []).toStmt(); - }); - } + const calls = bindings.map( + property => [o.literal(property.name), property.value(), ...(property.params || [])]); - private propertyInstructionChain( - nodeIndex: number, propertyBindings: ChainablePropertyBinding[]) { - this.updateInstructionChain( - nodeIndex, propertyBindings.length ? propertyBindings[0].input.sourceSpan : null, - R3.property, () => { - return propertyBindings.map( - property => [o.literal(property.name), property.value(), ...(property.params || [])]); - }); + return chainedInstruction(span, reference, calls).toStmt(); + }); } private addSelectInstructionIfNecessary(nodeIndex: number, span: ParseSourceSpan|null) { @@ -2024,7 +2026,7 @@ function hasTextChildrenOnly(children: t.Node[]): boolean { return children.every(isTextNode); } -interface ChainablePropertyBinding { +interface ChainableBindingInstruction { name: string; input: t.BoundAttribute; value: () => o.Expression; diff --git a/packages/core/src/render3/instructions/attribute.ts b/packages/core/src/render3/instructions/attribute.ts index 8d96076b68..229e3efa26 100644 --- a/packages/core/src/render3/instructions/attribute.ts +++ b/packages/core/src/render3/instructions/attribute.ts @@ -10,7 +10,7 @@ import {getLView, getSelectedIndex} from '../state'; import {NO_CHANGE} from '../tokens'; import {bind} from './property'; -import {elementAttributeInternal} from './shared'; +import {TsickleIssue1009, elementAttributeInternal} from './shared'; @@ -28,12 +28,14 @@ import {elementAttributeInternal} from './shared'; * @codeGenApi */ export function ɵɵattribute( - name: string, value: any, sanitizer?: SanitizerFn | null, namespace?: string) { + name: string, value: any, sanitizer?: SanitizerFn | null, + namespace?: string): TsickleIssue1009 { const index = getSelectedIndex(); const lView = getLView(); // TODO(FW-1340): Refactor to remove the use of other instructions here. const bound = bind(lView, value); if (bound !== NO_CHANGE) { - return elementAttributeInternal(index, name, bound, lView, sanitizer, namespace); + elementAttributeInternal(index, name, bound, lView, sanitizer, namespace); } + return ɵɵattribute; } diff --git a/packages/core/test/acceptance/attributes_spec.ts b/packages/core/test/acceptance/attributes_spec.ts index 1773227951..8b4794df03 100644 --- a/packages/core/test/acceptance/attributes_spec.ts +++ b/packages/core/test/acceptance/attributes_spec.ts @@ -73,6 +73,104 @@ describe('attribute binding', () => { expect(a.href).toEqual('https://angular.io/robots.txt'); }); + it('should be able to bind multiple attribute values per element', () => { + @Component({ + template: ``, + }) + class Comp { + url = 'https://angular.io/robots.txt'; + id = 'my-link'; + } + + TestBed.configureTestingModule({declarations: [Comp]}); + const fixture = TestBed.createComponent(Comp); + fixture.detectChanges(); + + const a = fixture.debugElement.query(By.css('a')).nativeElement; + // NOTE: different browsers will add `//` into the URI. + expect(a.getAttribute('href')).toBe('https://angular.io/robots.txt'); + expect(a.getAttribute('id')).toBe('my-link'); + expect(a.getAttribute('tabindex')).toBe('-1'); + }); + + it('should be able to bind multiple attributes in the presence of other bindings', () => { + @Component({ + template: ``, + }) + class Comp { + url = 'https://angular.io/robots.txt'; + id = 'my-link'; + } + + TestBed.configureTestingModule({declarations: [Comp]}); + const fixture = TestBed.createComponent(Comp); + fixture.detectChanges(); + + const a = fixture.debugElement.query(By.css('a')).nativeElement; + // NOTE: different browsers will add `//` into the URI. + expect(a.getAttribute('href')).toBe('https://angular.io/robots.txt'); + expect(a.id).toBe('my-link'); + expect(a.getAttribute('title')).toBe('hello'); + }); + + it('should be able to bind attributes with interpolations', () => { + @Component({ + template: ` + `, + }) + class Comp { + title = 'hello'; + id = 'custom'; + } + + TestBed.configureTestingModule({declarations: [Comp]}); + const fixture = TestBed.createComponent(Comp); + fixture.detectChanges(); + + const button = fixture.debugElement.query(By.css('button')).nativeElement; + + expect(button.getAttribute('id')).toBe('my-custom-button'); + expect(button.getAttribute('tabindex')).toBe('11'); + expect(button.getAttribute('title')).toBe('hello'); + }); + + + it('should be able to bind attributes both to parent and child nodes', () => { + @Component({ + template: ` + + `, + }) + class Comp { + title = 'hello'; + id = 'custom'; + } + + TestBed.configureTestingModule({declarations: [Comp]}); + const fixture = TestBed.createComponent(Comp); + fixture.detectChanges(); + + const button = fixture.debugElement.query(By.css('button')).nativeElement; + const span = fixture.debugElement.query(By.css('span')).nativeElement; + + expect(button.getAttribute('id')).toBe('my-custom-button'); + expect(button.getAttribute('tabindex')).toBe('11'); + expect(button.getAttribute('title')).toBe('hello'); + + expect(span.getAttribute('id')).toBe('custom-span'); + expect(span.getAttribute('tabindex')).toBe('-1'); + expect(span.getAttribute('title')).toBe('span-hello'); + }); + it('should sanitize attribute values', () => { @Component({ template: ``, diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index 2d906545a4..5048d8a5f6 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -665,7 +665,7 @@ export interface OutputDecorator { export declare function ɵɵallocHostVars(count: number): void; -export declare function ɵɵattribute(name: string, value: any, sanitizer?: SanitizerFn | null, namespace?: string): void; +export declare function ɵɵattribute(name: string, value: any, sanitizer?: SanitizerFn | null, namespace?: string): TsickleIssue1009; export declare function ɵɵattributeInterpolate1(attrName: string, prefix: string, v0: any, suffix: string, sanitizer?: SanitizerFn, namespace?: string): TsickleIssue1009;