refactor(render): don’t use a global cache for property setters

Related to #2359

modules/angular2/src/render/dom/compiler/compiler.ts

@@ -16,20 +16,25 @@ import {TemplateLoader} from 'angular2/src/render/dom/compiler/template_loader';
 import {CompileStepFactory, DefaultStepFactory} from './compile_step_factory';
 import {Parser} from 'angular2/change_detection';
 import {ShadowDomStrategy} from '../shadow_dom/shadow_dom_strategy';
+import {
+  PropertySetterFactory
+} from '../view/property_setter_factory'
 
-/**
+    /**
- * The compiler loads and translates the html templates of components into
+     * The compiler loads and translates the html templates of components into
- * nested ProtoViews. To decompose its functionality it uses
+     * nested ProtoViews. To decompose its functionality it uses
- * the CompilePipeline and the CompileSteps.
+     * the CompilePipeline and the CompileSteps.
- */
+     */
-export class DomCompiler extends RenderCompiler {
+    export class DomCompiler extends RenderCompiler {
   _templateLoader: TemplateLoader;
   _stepFactory: CompileStepFactory;
+  _propertySetterFactory: PropertySetterFactory;
 
   constructor(stepFactory: CompileStepFactory, templateLoader: TemplateLoader) {
     super();
     this._templateLoader = templateLoader;
     this._stepFactory = stepFactory;
+    this._propertySetterFactory = new PropertySetterFactory();
   }
 
   compile(template: ViewDefinition): Promise<ProtoViewDto> {
@@ -58,7 +63,7 @@ export class DomCompiler extends RenderCompiler {
     var pipeline = new CompilePipeline(this._stepFactory.createSteps(viewDef, subTaskPromises));
     var compileElements = pipeline.process(tplElement, protoViewType, viewDef.componentId);
 
-    var protoView = compileElements[0].inheritedProtoView.build();
+    var protoView = compileElements[0].inheritedProtoView.build(this._propertySetterFactory);
 
     if (subTaskPromises.length > 0) {
       return PromiseWrapper.all(subTaskPromises).then((_) => protoView);

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

@@ -13,119 +13,123 @@ import {camelCaseToDashCase, dashCaseToCamelCase} from '../util';
 import {reflector} from 'angular2/src/reflection/reflection';
 
 const STYLE_SEPARATOR = '.';
-var propertySettersCache = StringMapWrapper.create();
-var innerHTMLSetterCache;
 const ATTRIBUTE_PREFIX = 'attr.';
-var attributeSettersCache = StringMapWrapper.create();
 const CLASS_PREFIX = 'class.';
-var classSettersCache = StringMapWrapper.create();
 const STYLE_PREFIX = 'style.';
-var styleSettersCache = StringMapWrapper.create();
 
-export function setterFactory(property: string): Function {
+export class PropertySetterFactory {
-  var setterFn, styleParts, styleSuffix;
+  private _propertySettersCache: StringMap<string, Function> = StringMapWrapper.create();
-  if (StringWrapper.startsWith(property, ATTRIBUTE_PREFIX)) {
+  private _innerHTMLSetterCache: Function;
-    setterFn = attributeSetterFactory(StringWrapper.substring(property, ATTRIBUTE_PREFIX.length));
+  private _attributeSettersCache: StringMap<string, Function> = StringMapWrapper.create();
-  } else if (StringWrapper.startsWith(property, CLASS_PREFIX)) {
+  private _classSettersCache: StringMap<string, Function> = StringMapWrapper.create();
-    setterFn = classSetterFactory(StringWrapper.substring(property, CLASS_PREFIX.length));
+  private _styleSettersCache: StringMap<string, Function> = StringMapWrapper.create();
-  } else if (StringWrapper.startsWith(property, STYLE_PREFIX)) {
+
-    styleParts = property.split(STYLE_SEPARATOR);
+  createSetter(property: string): Function {
-    styleSuffix = styleParts.length > 2 ? ListWrapper.get(styleParts, 2) : '';
+    var setterFn, styleParts, styleSuffix;
-    setterFn = styleSetterFactory(ListWrapper.get(styleParts, 1), styleSuffix);
+    if (StringWrapper.startsWith(property, ATTRIBUTE_PREFIX)) {
-  } else if (StringWrapper.equals(property, 'innerHtml')) {
+      setterFn =
-    if (isBlank(innerHTMLSetterCache)) {
+          this._attributeSetterFactory(StringWrapper.substring(property, ATTRIBUTE_PREFIX.length));
-      innerHTMLSetterCache = (el, value) => DOM.setInnerHTML(el, value);
+    } else if (StringWrapper.startsWith(property, CLASS_PREFIX)) {
+      setterFn = this._classSetterFactory(StringWrapper.substring(property, CLASS_PREFIX.length));
+    } else if (StringWrapper.startsWith(property, STYLE_PREFIX)) {
+      styleParts = property.split(STYLE_SEPARATOR);
+      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);
+      if (isBlank(setterFn)) {
+        var propertySetterFn = reflector.setter(property);
+        setterFn = (receiver, value) => {
+          if (DOM.hasProperty(receiver, property)) {
+            return propertySetterFn(receiver, value);
+          }
+        };
+        StringMapWrapper.set(this._propertySettersCache, property, setterFn);
+      }
     }
-    setterFn = innerHTMLSetterCache;
+    return setterFn;
-  } else {
+  }
-    property = resolvePropertyName(property);
+
-    setterFn = StringMapWrapper.get(propertySettersCache, property);
+  private _isValidAttributeValue(attrName: string, value: any): boolean {
+    if (attrName == "role") {
+      return isString(value);
+    } else {
+      return isPresent(value);
+    }
+  }
+
+  private _attributeSetterFactory(attrName: string): Function {
+    var setterFn = StringMapWrapper.get(this._attributeSettersCache, attrName);
+    var dashCasedAttributeName;
+
     if (isBlank(setterFn)) {
-      var propertySetterFn = reflector.setter(property);
+      dashCasedAttributeName = camelCaseToDashCase(attrName);
-      setterFn = function(receiver, value) {
+      setterFn = (element, value) => {
-        if (DOM.hasProperty(receiver, property)) {
+        if (this._isValidAttributeValue(dashCasedAttributeName, value)) {
-          return propertySetterFn(receiver, value);
+          DOM.setAttribute(element, dashCasedAttributeName, stringify(value));
+        } else {
+          if (isPresent(value)) {
+            throw new BaseException("Invalid " + dashCasedAttributeName +
+                                    " attribute, only string values are allowed, got '" +
+                                    stringify(value) + "'");
+          }
+          DOM.removeAttribute(element, dashCasedAttributeName);
         }
       };
-      StringMapWrapper.set(propertySettersCache, property, setterFn);
+      StringMapWrapper.set(this._attributeSettersCache, attrName, setterFn);
     }
+
+    return setterFn;
   }
-  return setterFn;
-}
 
-function _isValidAttributeValue(attrName: string, value: any): boolean {
+  private _classSetterFactory(className: string): Function {
-  if (attrName == "role") {
+    var setterFn = StringMapWrapper.get(this._classSettersCache, className);
-    return isString(value);
+    var dashCasedClassName;
-  } else {
+    if (isBlank(setterFn)) {
-    return isPresent(value);
+      dashCasedClassName = camelCaseToDashCase(className);
-  }
+      setterFn = (element, value) => {
-}
+        if (value) {
-
+          DOM.addClass(element, dashCasedClassName);
-function attributeSetterFactory(attrName: string): Function {
+        } else {
-  var setterFn = StringMapWrapper.get(attributeSettersCache, attrName);
+          DOM.removeClass(element, dashCasedClassName);
-  var dashCasedAttributeName;
-
-  if (isBlank(setterFn)) {
-    dashCasedAttributeName = camelCaseToDashCase(attrName);
-    setterFn = function(element, value) {
-      if (_isValidAttributeValue(dashCasedAttributeName, value)) {
-        DOM.setAttribute(element, dashCasedAttributeName, stringify(value));
-      } else {
-        if (isPresent(value)) {
-          throw new BaseException("Invalid " + dashCasedAttributeName +
-                                  " attribute, only string values are allowed, got '" +
-                                  stringify(value) + "'");
         }
-        DOM.removeAttribute(element, dashCasedAttributeName);
+      };
-      }
+      StringMapWrapper.set(this._classSettersCache, className, setterFn);
-    };
+    }
-    StringMapWrapper.set(attributeSettersCache, attrName, setterFn);
+
+    return setterFn;
   }
 
-  return setterFn;
+  private _styleSetterFactory(styleName: string, styleSuffix: string): Function {
-}
+    var cacheKey = styleName + styleSuffix;
+    var setterFn = StringMapWrapper.get(this._styleSettersCache, cacheKey);
+    var dashCasedStyleName;
 
-function classSetterFactory(className: string): Function {
+    if (isBlank(setterFn)) {
-  var setterFn = StringMapWrapper.get(classSettersCache, className);
+      dashCasedStyleName = camelCaseToDashCase(styleName);
-  var dashCasedClassName;
+      setterFn = (element, value) => {
-  if (isBlank(setterFn)) {
+        var valAsStr;
-    dashCasedClassName = camelCaseToDashCase(className);
+        if (isPresent(value)) {
-    setterFn = function(element, value) {
+          valAsStr = stringify(value);
-      if (value) {
+          DOM.setStyle(element, dashCasedStyleName, valAsStr + styleSuffix);
-        DOM.addClass(element, dashCasedClassName);
+        } else {
-      } else {
+          DOM.removeStyle(element, dashCasedStyleName);
-        DOM.removeClass(element, dashCasedClassName);
+        }
-      }
+      };
-    };
+      StringMapWrapper.set(this._styleSettersCache, cacheKey, setterFn);
-    StringMapWrapper.set(classSettersCache, className, setterFn);
+    }
+
+    return setterFn;
   }
 
-  return setterFn;
+  private _resolvePropertyName(attrName: string): string {
-}
+    var mappedPropName = StringMapWrapper.get(DOM.attrToPropMap, attrName);
-
+    return isPresent(mappedPropName) ? mappedPropName : attrName;
-function styleSetterFactory(styleName: string, styleSuffix: string): Function {
-  var cacheKey = styleName + styleSuffix;
-  var setterFn = StringMapWrapper.get(styleSettersCache, cacheKey);
-  var dashCasedStyleName;
-
-  if (isBlank(setterFn)) {
-    dashCasedStyleName = camelCaseToDashCase(styleName);
-    setterFn = function(element, value) {
-      var valAsStr;
-      if (isPresent(value)) {
-        valAsStr = stringify(value);
-        DOM.setStyle(element, dashCasedStyleName, valAsStr + styleSuffix);
-      } else {
-        DOM.removeStyle(element, dashCasedStyleName);
-      }
-    };
-    StringMapWrapper.set(styleSettersCache, cacheKey, setterFn);
   }
-
-  return setterFn;
-}
-
-function resolvePropertyName(attrName: string): string {
-  var mappedPropName = StringMapWrapper.get(DOM.attrToPropMap, attrName);
-  return isPresent(mappedPropName) ? mappedPropName : attrName;
 }

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

@@ -13,24 +13,17 @@ import {
 
 import {DomProtoView, DomProtoViewRef, resolveInternalDomProtoView} from './proto_view';
 import {ElementBinder, Event, HostAction} from './element_binder';
-import {setterFactory} from './property_setter_factory';
+import {PropertySetterFactory} from './property_setter_factory';
 
 import * as api from '../../api';
 
 import {NG_BINDING_CLASS, EVENT_TARGET_SEPARATOR} from '../util';
 
 export class ProtoViewBuilder {
-  rootElement;
+  variableBindings: Map<string, string> = MapWrapper.create();
-  variableBindings: Map<string, string>;
+  elements: List<ElementBinderBuilder> = [];
-  elements: List<ElementBinderBuilder>;
-  type: number;
 
-  constructor(rootElement, type: number) {
+  constructor(public rootElement, public type: number) {}
-    this.rootElement = rootElement;
-    this.elements = [];
-    this.variableBindings = MapWrapper.create();
-    this.type = type;
-  }
 
   bindElement(element, description = null): ElementBinderBuilder {
     var builder = new ElementBinderBuilder(this.elements.length, element, description);
@@ -50,7 +43,7 @@ export class ProtoViewBuilder {
     MapWrapper.set(this.variableBindings, value, name);
   }
 
-  build(): api.ProtoViewDto {
+  build(setterFactory: PropertySetterFactory): api.ProtoViewDto {
     var renderElementBinders = [];
 
     var apiElementBinders = [];
@@ -63,7 +56,8 @@ export class ProtoViewBuilder {
         ebb.eventBuilder.merge(dbb.eventBuilder);
 
         MapWrapper.forEach(dbb.hostPropertyBindings, (_, hostPropertyName) => {
-          MapWrapper.set(propertySetters, hostPropertyName, setterFactory(hostPropertyName));
+          MapWrapper.set(propertySetters, hostPropertyName,
+                         setterFactory.createSetter(hostPropertyName));
         });
 
         ListWrapper.forEach(dbb.hostActions, (hostAction) => {
@@ -79,10 +73,11 @@ export class ProtoViewBuilder {
       });
 
       MapWrapper.forEach(ebb.propertyBindings, (_, propertyName) => {
-        MapWrapper.set(propertySetters, propertyName, setterFactory(propertyName));
+        MapWrapper.set(propertySetters, propertyName, setterFactory.createSetter(propertyName));
       });
 
-      var nestedProtoView = isPresent(ebb.nestedProtoView) ? ebb.nestedProtoView.build() : null;
+      var nestedProtoView =
+          isPresent(ebb.nestedProtoView) ? ebb.nestedProtoView.build(setterFactory) : null;
       var nestedRenderProtoView =
           isPresent(nestedProtoView) ? resolveInternalDomProtoView(nestedProtoView.render) : null;
       if (isPresent(nestedRenderProtoView)) {

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

@@ -9,25 +9,28 @@ import {
   beforeEach,
   el
 } from 'angular2/test_lib';
-import {setterFactory} from 'angular2/src/render/dom/view/property_setter_factory';
+import {PropertySetterFactory} from 'angular2/src/render/dom/view/property_setter_factory';
 import {DOM} from 'angular2/src/dom/dom_adapter';
 
 export function main() {
-  var div;
+  var div, setterFactory;
-  beforeEach(() => { div = el('<div></div>'); });
+  beforeEach(() => {
+    div = el('<div></div>');
+    setterFactory = new PropertySetterFactory();
+  });
   describe('property setter factory', () => {
 
     it('should return a setter for a property', () => {
-      var setterFn = setterFactory('title');
+      var setterFn = setterFactory.createSetter('title');
       setterFn(div, 'Hello');
       expect(div.title).toEqual('Hello');
 
-      var otherSetterFn = setterFactory('title');
+      var otherSetterFn = setterFactory.createSetter('title');
       expect(setterFn).toBe(otherSetterFn);
     });
 
     it('should return a setter for an attribute', () => {
-      var setterFn = setterFactory('attr.role');
+      var setterFn = setterFactory.createSetter('attr.role');
       setterFn(div, 'button');
       expect(DOM.getAttribute(div, 'role')).toEqual('button');
       setterFn(div, null);
@@ -35,49 +38,49 @@ export function main() {
       expect(() => { setterFn(div, 4); })
           .toThrowError("Invalid role attribute, only string values are allowed, got '4'");
 
-      var otherSetterFn = setterFactory('attr.role');
+      var otherSetterFn = setterFactory.createSetter('attr.role');
       expect(setterFn).toBe(otherSetterFn);
     });
 
     it('should return a setter for a class', () => {
-      var setterFn = setterFactory('class.active');
+      var setterFn = setterFactory.createSetter('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('class.active');
+      var otherSetterFn = setterFactory.createSetter('class.active');
       expect(setterFn).toBe(otherSetterFn);
     });
 
     it('should return a setter for a style', () => {
-      var setterFn = setterFactory('style.width');
+      var setterFn = setterFactory.createSetter('style.width');
       setterFn(div, '40px');
       expect(DOM.getStyle(div, 'width')).toEqual('40px');
       setterFn(div, null);
       expect(DOM.getStyle(div, 'width')).toEqual('');
 
-      var otherSetterFn = setterFactory('style.width');
+      var otherSetterFn = setterFactory.createSetter('style.width');
       expect(setterFn).toBe(otherSetterFn);
     });
 
     it('should return a setter for a style with a unit', () => {
-      var setterFn = setterFactory('style.height.px');
+      var setterFn = setterFactory.createSetter('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('style.height.px');
+      var otherSetterFn = setterFactory.createSetter('style.height.px');
       expect(setterFn).toBe(otherSetterFn);
     });
 
     it('should return a setter for innerHtml', () => {
-      var setterFn = setterFactory('innerHtml');
+      var setterFn = setterFactory.createSetter('innerHtml');
       setterFn(div, '<span></span>');
       expect(DOM.getInnerHTML(div)).toEqual('<span></span>');
 
-      var otherSetterFn = setterFactory('innerHtml');
+      var otherSetterFn = setterFactory.createSetter('innerHtml');
       expect(setterFn).toBe(otherSetterFn);
     });