From 4f27611ae66258bda233b77602cc9b84d78cfd5a Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Wed, 10 Jun 2015 10:31:38 -0700 Subject: [PATCH] perf(render): don't create property setters if not needed --- modules/angular2/src/dom/html_adapter.dart | 5 +- modules/angular2/src/dom/parse5_adapter.ts | 2 +- .../dom/view/property_setter_factory.ts | 37 ++++++++-- .../src/render/dom/view/proto_view_builder.ts | 4 +- .../dom/view/property_setter_factory_spec.ts | 68 ++++++++++++++----- 5 files changed, 87 insertions(+), 29 deletions(-) diff --git a/modules/angular2/src/dom/html_adapter.dart b/modules/angular2/src/dom/html_adapter.dart index 18bfd0e08e..86a5573236 100644 --- a/modules/angular2/src/dom/html_adapter.dart +++ b/modules/angular2/src/dom/html_adapter.dart @@ -142,7 +142,7 @@ class Html5LibDomAdapter implements DomAdapter { throw 'not implemented'; } getText(el) { - throw 'not implemented'; + return el.text; } setText(el, String value) => el.text = value; @@ -183,7 +183,8 @@ class Html5LibDomAdapter implements DomAdapter { clone(node) => node.clone(true); hasProperty(element, String name) { - throw 'not implemented'; + // This is needed for serverside compile to generate the right getters/setters... + return true; } getElementsByClassName(element, String name) { throw 'not implemented'; diff --git a/modules/angular2/src/dom/parse5_adapter.ts b/modules/angular2/src/dom/parse5_adapter.ts index d1f0f0f308..67ae4d6b83 100644 --- a/modules/angular2/src/dom/parse5_adapter.ts +++ b/modules/angular2/src/dom/parse5_adapter.ts @@ -205,7 +205,7 @@ export class Parse5DomAdapter extends DomAdapter { getText(el) { if (this.isTextNode(el)) { return el.data; - } else if (el.childNodes.length == 0) { + } else if (isBlank(el.childNodes) || el.childNodes.length == 0) { return ""; } else { var textContent = ""; diff --git a/modules/angular2/src/render/dom/view/property_setter_factory.ts b/modules/angular2/src/render/dom/view/property_setter_factory.ts index 6f13174ab2..bd70fde45f 100644 --- a/modules/angular2/src/render/dom/view/property_setter_factory.ts +++ b/modules/angular2/src/render/dom/view/property_setter_factory.ts @@ -18,13 +18,18 @@ const CLASS_PREFIX = 'class.'; const STYLE_PREFIX = 'style.'; export class PropertySetterFactory { - private _propertySettersCache: StringMap = StringMapWrapper.create(); + private static _noopSetter(el, value) {} + + private _lazyPropertySettersCache: StringMap = StringMapWrapper.create(); + private _eagerPropertySettersCache: StringMap = StringMapWrapper.create(); private _innerHTMLSetterCache: Function; private _attributeSettersCache: StringMap = StringMapWrapper.create(); private _classSettersCache: StringMap = StringMapWrapper.create(); private _styleSettersCache: StringMap = StringMapWrapper.create(); - createSetter(property: string): Function { + constructor() { this._innerHTMLSetterCache = (el, value) => DOM.setInnerHTML(el, value); } + + createSetter(protoElement: /*element*/ any, isNgComponent: boolean, property: string): Function { var setterFn, styleParts, styleSuffix; if (StringWrapper.startsWith(property, ATTRIBUTE_PREFIX)) { setterFn = @@ -36,13 +41,21 @@ export class PropertySetterFactory { styleSuffix = styleParts.length > 2 ? ListWrapper.get(styleParts, 2) : ''; setterFn = this._styleSetterFactory(ListWrapper.get(styleParts, 1), styleSuffix); } else if (StringWrapper.equals(property, 'innerHtml')) { - if (isBlank(this._innerHTMLSetterCache)) { - this._innerHTMLSetterCache = (el, value) => DOM.setInnerHTML(el, value); - } setterFn = this._innerHTMLSetterCache; } else { property = this._resolvePropertyName(property); - setterFn = StringMapWrapper.get(this._propertySettersCache, property); + setterFn = this._propertySetterFactory(protoElement, isNgComponent, property); + } + return setterFn; + } + + private _propertySetterFactory(protoElement, isNgComponent: boolean, property: string): Function { + var setterFn; + var tagName = DOM.tagName(protoElement); + var possibleCustomElement = tagName.indexOf('-') !== -1; + if (possibleCustomElement && !isNgComponent) { + // need to use late check to be able to set properties on custom elements + setterFn = StringMapWrapper.get(this._lazyPropertySettersCache, property); if (isBlank(setterFn)) { var propertySetterFn = reflector.setter(property); setterFn = (receiver, value) => { @@ -50,7 +63,17 @@ export class PropertySetterFactory { return propertySetterFn(receiver, value); } }; - StringMapWrapper.set(this._propertySettersCache, property, setterFn); + StringMapWrapper.set(this._lazyPropertySettersCache, property, setterFn); + } + } else { + setterFn = StringMapWrapper.get(this._eagerPropertySettersCache, property); + if (isBlank(setterFn)) { + if (DOM.hasProperty(protoElement, property)) { + setterFn = reflector.setter(property); + } else { + setterFn = PropertySetterFactory._noopSetter; + } + StringMapWrapper.set(this._eagerPropertySettersCache, property, setterFn); } } return setterFn; diff --git a/modules/angular2/src/render/dom/view/proto_view_builder.ts b/modules/angular2/src/render/dom/view/proto_view_builder.ts index 68fb3e549f..d838167ff7 100644 --- a/modules/angular2/src/render/dom/view/proto_view_builder.ts +++ b/modules/angular2/src/render/dom/view/proto_view_builder.ts @@ -57,7 +57,7 @@ export class ProtoViewBuilder { MapWrapper.forEach(dbb.hostPropertyBindings, (_, hostPropertyName) => { MapWrapper.set(propertySetters, hostPropertyName, - setterFactory.createSetter(hostPropertyName)); + setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), hostPropertyName)); }); ListWrapper.forEach(dbb.hostActions, (hostAction) => { @@ -73,7 +73,7 @@ export class ProtoViewBuilder { }); MapWrapper.forEach(ebb.propertyBindings, (_, propertyName) => { - MapWrapper.set(propertySetters, propertyName, setterFactory.createSetter(propertyName)); + MapWrapper.set(propertySetters, propertyName, setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName)); }); var nestedProtoView = diff --git a/modules/angular2/test/render/dom/view/property_setter_factory_spec.ts b/modules/angular2/test/render/dom/view/property_setter_factory_spec.ts index fa9fb9cc2b..9ec9102eb5 100644 --- a/modules/angular2/test/render/dom/view/property_setter_factory_spec.ts +++ b/modules/angular2/test/render/dom/view/property_setter_factory_spec.ts @@ -7,7 +7,8 @@ import { xdescribe, expect, beforeEach, - el + el, + IS_DARTIUM } from 'angular2/test_lib'; import {PropertySetterFactory} from 'angular2/src/render/dom/view/property_setter_factory'; import {DOM} from 'angular2/src/dom/dom_adapter'; @@ -20,17 +21,50 @@ export function main() { }); describe('property setter factory', () => { - it('should return a setter for a property', () => { - var setterFn = setterFactory.createSetter('title'); - setterFn(div, 'Hello'); - expect(div.title).toEqual('Hello'); + describe('property setters', () => { + + it('should set an existing property', () => { + var setterFn = setterFactory.createSetter(div, false, 'title'); + setterFn(div, 'Hello'); + expect(div.title).toEqual('Hello'); + + var otherSetterFn = setterFactory.createSetter(div, false, 'title'); + expect(setterFn).toBe(otherSetterFn); + }); + + if (!IS_DARTIUM) { + it('should use a noop setter if the property did not exist when the setter was created', () => { + var setterFn = setterFactory.createSetter(div, false, 'someProp'); + div.someProp = ''; + setterFn(div, 'Hello'); + expect(div.someProp).toEqual(''); + }); + + it('should use a noop setter if the property did not exist when the setter was created for ng components', () => { + var ce = el(''); + var setterFn = setterFactory.createSetter(ce, true, 'someProp'); + ce.someProp = ''; + setterFn(ce, 'Hello'); + expect(ce.someProp).toEqual(''); + }); + + it('should set the property for custom elements even if it was not present when the setter was created', () => { + var ce = el(''); + var setterFn = setterFactory.createSetter(ce, false, 'someProp'); + ce.someProp = ''; + // Our CJS DOM adapter does not support custom properties, + // need to exclude here. + if (DOM.hasProperty(ce, 'someProp')) { + setterFn(ce, 'Hello'); + expect(ce.someProp).toEqual('Hello'); + } + }); + } - var otherSetterFn = setterFactory.createSetter('title'); - expect(setterFn).toBe(otherSetterFn); }); it('should return a setter for an attribute', () => { - var setterFn = setterFactory.createSetter('attr.role'); + var setterFn = setterFactory.createSetter(div, false, 'attr.role'); setterFn(div, 'button'); expect(DOM.getAttribute(div, 'role')).toEqual('button'); setterFn(div, null); @@ -38,49 +72,49 @@ export function main() { expect(() => { setterFn(div, 4); }) .toThrowError("Invalid role attribute, only string values are allowed, got '4'"); - var otherSetterFn = setterFactory.createSetter('attr.role'); + var otherSetterFn = setterFactory.createSetter(div, false, 'attr.role'); expect(setterFn).toBe(otherSetterFn); }); it('should return a setter for a class', () => { - var setterFn = setterFactory.createSetter('class.active'); + var setterFn = setterFactory.createSetter(div, false, 'class.active'); setterFn(div, true); expect(DOM.hasClass(div, 'active')).toEqual(true); setterFn(div, false); expect(DOM.hasClass(div, 'active')).toEqual(false); - var otherSetterFn = setterFactory.createSetter('class.active'); + var otherSetterFn = setterFactory.createSetter(div, false, 'class.active'); expect(setterFn).toBe(otherSetterFn); }); it('should return a setter for a style', () => { - var setterFn = setterFactory.createSetter('style.width'); + var setterFn = setterFactory.createSetter(div, false, 'style.width'); setterFn(div, '40px'); expect(DOM.getStyle(div, 'width')).toEqual('40px'); setterFn(div, null); expect(DOM.getStyle(div, 'width')).toEqual(''); - var otherSetterFn = setterFactory.createSetter('style.width'); + var otherSetterFn = setterFactory.createSetter(div, false, 'style.width'); expect(setterFn).toBe(otherSetterFn); }); it('should return a setter for a style with a unit', () => { - var setterFn = setterFactory.createSetter('style.height.px'); + var setterFn = setterFactory.createSetter(div, false, 'style.height.px'); setterFn(div, 40); expect(DOM.getStyle(div, 'height')).toEqual('40px'); setterFn(div, null); expect(DOM.getStyle(div, 'height')).toEqual(''); - var otherSetterFn = setterFactory.createSetter('style.height.px'); + var otherSetterFn = setterFactory.createSetter(div, false, 'style.height.px'); expect(setterFn).toBe(otherSetterFn); }); it('should return a setter for innerHtml', () => { - var setterFn = setterFactory.createSetter('innerHtml'); + var setterFn = setterFactory.createSetter(div, false, 'innerHtml'); setterFn(div, ''); expect(DOM.getInnerHTML(div)).toEqual(''); - var otherSetterFn = setterFactory.createSetter('innerHtml'); + var otherSetterFn = setterFactory.createSetter(div, false, 'innerHtml'); expect(setterFn).toBe(otherSetterFn); });