X Tutup
Skip to content

Commit 4f27611

Browse files
tboschtbosch
committed
perf(render): don't create property setters if not needed
1 parent 24e647e commit 4f27611

File tree

5 files changed

+87
-29
lines changed

5 files changed

+87
-29
lines changed

‎modules/angular2/src/dom/html_adapter.dart‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ class Html5LibDomAdapter implements DomAdapter {
142142
throw 'not implemented';
143143
}
144144
getText(el) {
145-
throw 'not implemented';
145+
return el.text;
146146
}
147147
setText(el, String value) => el.text = value;
148148

@@ -183,7 +183,8 @@ class Html5LibDomAdapter implements DomAdapter {
183183
clone(node) => node.clone(true);
184184

185185
hasProperty(element, String name) {
186-
throw 'not implemented';
186+
// This is needed for serverside compile to generate the right getters/setters...
187+
return true;
187188
}
188189
getElementsByClassName(element, String name) {
189190
throw 'not implemented';

‎modules/angular2/src/dom/parse5_adapter.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -205,7 +205,7 @@ export class Parse5DomAdapter extends DomAdapter {
205205
getText(el) {
206206
if (this.isTextNode(el)) {
207207
return el.data;
208-
} else if (el.childNodes.length == 0) {
208+
} else if (isBlank(el.childNodes) || el.childNodes.length == 0) {
209209
return "";
210210
} else {
211211
var textContent = "";

‎modules/angular2/src/render/dom/view/property_setter_factory.ts‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,18 @@ const CLASS_PREFIX = 'class.';
1818
const STYLE_PREFIX = 'style.';
1919

2020
export class PropertySetterFactory {
21-
private _propertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
21+
private static _noopSetter(el, value) {}
22+
23+
private _lazyPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
24+
private _eagerPropertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
2225
private _innerHTMLSetterCache: Function;
2326
private _attributeSettersCache: StringMap<string, Function> = StringMapWrapper.create();
2427
private _classSettersCache: StringMap<string, Function> = StringMapWrapper.create();
2528
private _styleSettersCache: StringMap<string, Function> = StringMapWrapper.create();
2629

27-
createSetter(property: string): Function {
30+
constructor() { this._innerHTMLSetterCache = (el, value) => DOM.setInnerHTML(el, value); }
31+
32+
createSetter(protoElement: /*element*/ any, isNgComponent: boolean, property: string): Function {
2833
var setterFn, styleParts, styleSuffix;
2934
if (StringWrapper.startsWith(property, ATTRIBUTE_PREFIX)) {
3035
setterFn =
@@ -36,21 +41,39 @@ export class PropertySetterFactory {
3641
styleSuffix = styleParts.length > 2 ? ListWrapper.get(styleParts, 2) : '';
3742
setterFn = this._styleSetterFactory(ListWrapper.get(styleParts, 1), styleSuffix);
3843
} else if (StringWrapper.equals(property, 'innerHtml')) {
39-
if (isBlank(this._innerHTMLSetterCache)) {
40-
this._innerHTMLSetterCache = (el, value) => DOM.setInnerHTML(el, value);
41-
}
4244
setterFn = this._innerHTMLSetterCache;
4345
} else {
4446
property = this._resolvePropertyName(property);
45-
setterFn = StringMapWrapper.get(this._propertySettersCache, property);
47+
setterFn = this._propertySetterFactory(protoElement, isNgComponent, property);
48+
}
49+
return setterFn;
50+
}
51+
52+
private _propertySetterFactory(protoElement, isNgComponent: boolean, property: string): Function {
53+
var setterFn;
54+
var tagName = DOM.tagName(protoElement);
55+
var possibleCustomElement = tagName.indexOf('-') !== -1;
56+
if (possibleCustomElement && !isNgComponent) {
57+
// need to use late check to be able to set properties on custom elements
58+
setterFn = StringMapWrapper.get(this._lazyPropertySettersCache, property);
4659
if (isBlank(setterFn)) {
4760
var propertySetterFn = reflector.setter(property);
4861
setterFn = (receiver, value) => {
4962
if (DOM.hasProperty(receiver, property)) {
5063
return propertySetterFn(receiver, value);
5164
}
5265
};
53-
StringMapWrapper.set(this._propertySettersCache, property, setterFn);
66+
StringMapWrapper.set(this._lazyPropertySettersCache, property, setterFn);
67+
}
68+
} else {
69+
setterFn = StringMapWrapper.get(this._eagerPropertySettersCache, property);
70+
if (isBlank(setterFn)) {
71+
if (DOM.hasProperty(protoElement, property)) {
72+
setterFn = reflector.setter(property);
73+
} else {
74+
setterFn = PropertySetterFactory._noopSetter;
75+
}
76+
StringMapWrapper.set(this._eagerPropertySettersCache, property, setterFn);
5477
}
5578
}
5679
return setterFn;

‎modules/angular2/src/render/dom/view/proto_view_builder.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ export class ProtoViewBuilder {
5757

5858
MapWrapper.forEach(dbb.hostPropertyBindings, (_, hostPropertyName) => {
5959
MapWrapper.set(propertySetters, hostPropertyName,
60-
setterFactory.createSetter(hostPropertyName));
60+
setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), hostPropertyName));
6161
});
6262

6363
ListWrapper.forEach(dbb.hostActions, (hostAction) => {
@@ -73,7 +73,7 @@ export class ProtoViewBuilder {
7373
});
7474

7575
MapWrapper.forEach(ebb.propertyBindings, (_, propertyName) => {
76-
MapWrapper.set(propertySetters, propertyName, setterFactory.createSetter(propertyName));
76+
MapWrapper.set(propertySetters, propertyName, setterFactory.createSetter(ebb.element, isPresent(ebb.componentId), propertyName));
7777
});
7878

7979
var nestedProtoView =

‎modules/angular2/test/render/dom/view/property_setter_factory_spec.ts‎

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@ import {
77
xdescribe,
88
expect,
99
beforeEach,
10-
el
10+
el,
11+
IS_DARTIUM
1112
} from 'angular2/test_lib';
1213
import {PropertySetterFactory} from 'angular2/src/render/dom/view/property_setter_factory';
1314
import {DOM} from 'angular2/src/dom/dom_adapter';
@@ -20,67 +21,100 @@ export function main() {
2021
});
2122
describe('property setter factory', () => {
2223

23-
it('should return a setter for a property', () => {
24-
var setterFn = setterFactory.createSetter('title');
25-
setterFn(div, 'Hello');
26-
expect(div.title).toEqual('Hello');
24+
describe('property setters', () => {
25+
26+
it('should set an existing property', () => {
27+
var setterFn = setterFactory.createSetter(div, false, 'title');
28+
setterFn(div, 'Hello');
29+
expect(div.title).toEqual('Hello');
30+
31+
var otherSetterFn = setterFactory.createSetter(div, false, 'title');
32+
expect(setterFn).toBe(otherSetterFn);
33+
});
34+
35+
if (!IS_DARTIUM) {
36+
it('should use a noop setter if the property did not exist when the setter was created', () => {
37+
var setterFn = setterFactory.createSetter(div, false, 'someProp');
38+
div.someProp = '';
39+
setterFn(div, 'Hello');
40+
expect(div.someProp).toEqual('');
41+
});
42+
43+
it('should use a noop setter if the property did not exist when the setter was created for ng components', () => {
44+
var ce = el('<some-ce></some-ce>');
45+
var setterFn = setterFactory.createSetter(ce, true, 'someProp');
46+
ce.someProp = '';
47+
setterFn(ce, 'Hello');
48+
expect(ce.someProp).toEqual('');
49+
});
50+
51+
it('should set the property for custom elements even if it was not present when the setter was created', () => {
52+
var ce = el('<some-ce></some-ce>');
53+
var setterFn = setterFactory.createSetter(ce, false, 'someProp');
54+
ce.someProp = '';
55+
// Our CJS DOM adapter does not support custom properties,
56+
// need to exclude here.
57+
if (DOM.hasProperty(ce, 'someProp')) {
58+
setterFn(ce, 'Hello');
59+
expect(ce.someProp).toEqual('Hello');
60+
}
61+
});
62+
}
2763

28-
var otherSetterFn = setterFactory.createSetter('title');
29-
expect(setterFn).toBe(otherSetterFn);
3064
});
3165

3266
it('should return a setter for an attribute', () => {
33-
var setterFn = setterFactory.createSetter('attr.role');
67+
var setterFn = setterFactory.createSetter(div, false, 'attr.role');
3468
setterFn(div, 'button');
3569
expect(DOM.getAttribute(div, 'role')).toEqual('button');
3670
setterFn(div, null);
3771
expect(DOM.getAttribute(div, 'role')).toEqual(null);
3872
expect(() => { setterFn(div, 4); })
3973
.toThrowError("Invalid role attribute, only string values are allowed, got '4'");
4074

41-
var otherSetterFn = setterFactory.createSetter('attr.role');
75+
var otherSetterFn = setterFactory.createSetter(div, false, 'attr.role');
4276
expect(setterFn).toBe(otherSetterFn);
4377
});
4478

4579
it('should return a setter for a class', () => {
46-
var setterFn = setterFactory.createSetter('class.active');
80+
var setterFn = setterFactory.createSetter(div, false, 'class.active');
4781
setterFn(div, true);
4882
expect(DOM.hasClass(div, 'active')).toEqual(true);
4983
setterFn(div, false);
5084
expect(DOM.hasClass(div, 'active')).toEqual(false);
5185

52-
var otherSetterFn = setterFactory.createSetter('class.active');
86+
var otherSetterFn = setterFactory.createSetter(div, false, 'class.active');
5387
expect(setterFn).toBe(otherSetterFn);
5488
});
5589

5690
it('should return a setter for a style', () => {
57-
var setterFn = setterFactory.createSetter('style.width');
91+
var setterFn = setterFactory.createSetter(div, false, 'style.width');
5892
setterFn(div, '40px');
5993
expect(DOM.getStyle(div, 'width')).toEqual('40px');
6094
setterFn(div, null);
6195
expect(DOM.getStyle(div, 'width')).toEqual('');
6296

63-
var otherSetterFn = setterFactory.createSetter('style.width');
97+
var otherSetterFn = setterFactory.createSetter(div, false, 'style.width');
6498
expect(setterFn).toBe(otherSetterFn);
6599
});
66100

67101
it('should return a setter for a style with a unit', () => {
68-
var setterFn = setterFactory.createSetter('style.height.px');
102+
var setterFn = setterFactory.createSetter(div, false, 'style.height.px');
69103
setterFn(div, 40);
70104
expect(DOM.getStyle(div, 'height')).toEqual('40px');
71105
setterFn(div, null);
72106
expect(DOM.getStyle(div, 'height')).toEqual('');
73107

74-
var otherSetterFn = setterFactory.createSetter('style.height.px');
108+
var otherSetterFn = setterFactory.createSetter(div, false, 'style.height.px');
75109
expect(setterFn).toBe(otherSetterFn);
76110
});
77111

78112
it('should return a setter for innerHtml', () => {
79-
var setterFn = setterFactory.createSetter('innerHtml');
113+
var setterFn = setterFactory.createSetter(div, false, 'innerHtml');
80114
setterFn(div, '<span></span>');
81115
expect(DOM.getInnerHTML(div)).toEqual('<span></span>');
82116

83-
var otherSetterFn = setterFactory.createSetter('innerHtml');
117+
var otherSetterFn = setterFactory.createSetter(div, false, 'innerHtml');
84118
expect(setterFn).toBe(otherSetterFn);
85119
});
86120

0 commit comments

Comments
 (0)
X Tutup