[PropertiesMixin] Fix mapping property names from attributes

* make static: propertyNameForAttribute, attributeNameForProperty, typeForProperty * method that take classes should consistently use `constructor` as the arg name. * when using `properties`, the property name for a given attribute is discovered using the names in `properties` and reverse mapping the `attributeNameForProperty` attribute.
Polymer · Nov 20, 2017 · feac932 · feac932
1 parent c56f74f
commit feac932
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 90 deletions.
diff --git a/lib/mixins/element-mixin.html b/lib/mixins/element-mixin.html
@@ -105,41 +105,41 @@
      * the list returned from `_properties`.
      * This list is used in `_initializeProperties` to set property defaults.
      *
-     * @param {PolymerElementConstructor} klass Element class
+     * @param {PolymerElementConstructor} constructor Element class
      * @return {PolymerElementProperties} Flattened properties for this class
      *   that have default values
      * @private
      */
-    function propertyDefaults(klass) {
-      if (!klass.hasOwnProperty(
-        JSCompiler_renameProperty('__propertyDefaults', klass))) {
-        klass.__propertyDefaults = null;
-        let props = klass._properties;
+    function propertyDefaults(constructor) {
+      if (!constructor.hasOwnProperty(
+        JSCompiler_renameProperty('__propertyDefaults', constructor))) {
+        constructor.__propertyDefaults = null;
+        let props = constructor._properties;
         for (let p in props) {
           let info = props[p];
           if ('value' in info) {
-            klass.__propertyDefaults = klass.__propertyDefaults || {};
-            klass.__propertyDefaults[p] = info;
+            constructor.__propertyDefaults = constructor.__propertyDefaults || {};
+            constructor.__propertyDefaults[p] = info;
           }
         }
       }
-      return klass.__propertyDefaults;
+      return constructor.__propertyDefaults;
     }
 
     /**
      * Returns a memoized version of the the `observers` array.
-     * @param {PolymerElementConstructor} klass Element class
+     * @param {PolymerElementConstructor} constructor Element class
      * @return {Array} Array containing own observers for the given class
      * @protected
      */
-    function ownObservers(klass) {
-      if (!klass.hasOwnProperty(
-        JSCompiler_renameProperty('__ownObservers', klass))) {
-          klass.__ownObservers =
-          klass.hasOwnProperty(JSCompiler_renameProperty('observers', klass)) ?
-          /** @type {PolymerElementConstructor} */ (klass).observers : null;
+    function ownObservers(constructor) {
+      if (!constructor.hasOwnProperty(
+        JSCompiler_renameProperty('__ownObservers', constructor))) {
+          constructor.__ownObservers =
+          constructor.hasOwnProperty(JSCompiler_renameProperty('observers', constructor)) ?
+          /** @type {PolymerElementConstructor} */ (constructor).observers : null;
       }
-      return klass.__ownObservers;
+      return constructor.__ownObservers;
     }
 
     /**
@@ -561,7 +561,7 @@
        */
       attributeChangedCallback(name, old, value) {
         if (old !== value) {
-          let property = this._propertyNameForAttribute(name);
+          let property = this.constructor.propertyNameForAttribute(name);
           if (!this._hasReadOnlyEffect(property)) {
             super.attributeChangedCallback(name, old, value);
           }

diff --git a/lib/mixins/properties-changed.html b/lib/mixins/properties-changed.html
@@ -68,6 +68,39 @@
           }
         }
 
+        /**
+         * Returns a property name that corresponds to the given attribute.
+         * By default, converts dash to camel case, e.g. `foo-bar` to `fooBar`.
+         * @param {string} attribute Attribute to convert
+         * @return {string} Property name corresponding to the given attribute.
+         *
+         * @protected
+         */
+         static propertyNameForAttribute(attribute) {
+          return attribute;
+        }
+
+        /**
+         * Returns an attribute name that corresponds to the given property.
+         * By default, converts camel to dash case, e.g. `fooBar` to `foo-bar`.
+         * @param {string} property Property to convert
+         * @return {string} Attribute name corresponding to the given property.
+         *
+         * @protected
+         */
+         static attributeNameForProperty(property) {
+          return property.toLowerCase();
+        }
+
+        /**
+         * Override point to provide a type to which to deserialize a value to
+         * a given property.
+         * @param {string} name Name of property
+         *
+         * @protected
+         */
+        static typeForProperty(name) { } //eslint-disable-line no-unused-vars
+
         /**
          * Creates a setter/getter pair for the named property with its own
          * local storage.  The getter returns the value in the local storage,
@@ -355,13 +388,13 @@
          * @param {string} attribute Name of attribute to deserialize.
          * @param {?string} value of the attribute.
          * @param {*=} type type to deserialize to, defaults to the value
-         * returned from `_typeForProperty`
+         * returned from `typeForProperty`
          */
         _attributeToProperty(attribute, value, type) {
           if (!this.__serializing) {
-            const property = this._propertyNameForAttribute(attribute);
+            const property = this.constructor.propertyNameForAttribute(attribute);
             this[property] = this._deserializeValue(value, type ||
-              this._typeForProperty(property));
+              this.constructor.typeForProperty(property));
           }
         }
 
@@ -378,7 +411,7 @@
           this.__serializing = true;
           value = (arguments.length < 3) ? this[property] : value;
           this._valueToNodeAttribute(/** @type {!HTMLElement} */(this), value,
-            attribute || this._attributeNameForProperty(property));
+            attribute || this.constructor.attributeNameForProperty(property));
           this.__serializing = false;
         }
 
@@ -447,39 +480,6 @@
           }
         }
 
-        /**
-         * Returns a property name that corresponds to the given attribute.
-         * By default, converts dash to camel case, e.g. `foo-bar` to `fooBar`.
-         * @param {string} attribute Attribute to convert
-         * @return {string} Property name corresponding to the given attribute.
-         *
-         * @protected
-         */
-        _propertyNameForAttribute(attribute) {
-          return attribute;
-        }
-
-        /**
-         * Returns an attribute name that corresponds to the given property.
-         * By default, converts camel to dash case, e.g. `fooBar` to `foo-bar`.
-         * @param {string} property Property to convert
-         * @return {string} Attribute name corresponding to the given property.
-         *
-         * @protected
-         */
-         _attributeNameForProperty(property) {
-          return property.toLowerCase();
-        }
-
-        /**
-         * Override point to provide a type to which to deserialize a value to
-         * a given property.
-         * @param {string} name Name of property
-         *
-         * @protected
-         */
-        _typeForProperty(name) { } //eslint-disable-line no-unused-vars
-
       }
 
       return PropertiesChanged;

diff --git a/lib/mixins/properties-mixin.html b/lib/mixins/properties-mixin.html
@@ -67,11 +67,11 @@
      * Returns the super class constructor for the given class, if it is an
      * instance of the PropertiesClass.
      *
-     * @param {PropertiesClassConstructor} ctor PropertiesClass constructor
+     * @param {PropertiesClassConstructor} constructor PropertiesClass constructor
      * @return {PropertiesClassConstructor} Super class constructor
      */
-    function superForClass(ctor) {
-      const proto = /** @type {PropertiesClassConstructor} */ (ctor).prototype;
+    function superForClass(constructor) {
+      const proto = /** @type {PropertiesClassConstructor} */ (constructor).prototype;
       const superCtor = Object.getPrototypeOf(proto).constructor;
       if (superCtor.prototype instanceof PropertiesClass) {
         return superCtor;
@@ -83,31 +83,39 @@
      * given class. Properties not in object format are converted to at
      * least {type}.
      *
-     * @param {PropertiesClassConstructor} ctor PropertiesClass constructor
+     * @param {PropertiesClassConstructor} constructor PropertiesClass constructor
      * @return {Object} Memoized properties object
      */
-    function ownProperties(ctor) {
-      if (!ctor.hasOwnProperty(
-        JSCompiler_renameProperty('__ownProperties', ctor))) {
-        const props = ctor.properties;
-        ctor.__ownProperties = props ? normalizeProperties(props) : null;
+    function ownProperties(constructor) {
+      if (!constructor.hasOwnProperty(
+        JSCompiler_renameProperty('__ownProperties', constructor))) {
+        const props = constructor.properties;
+        constructor.__ownProperties = props ? normalizeProperties(props) : null;
       }
-      return ctor.__ownProperties;
+      return constructor.__ownProperties;
     }
 
     /**
-     * Returns map of properties for attributes for the given class.
+     * Creates a map matching attribute names to properties by using
+     * `attributeNameForProperty` and the names in `properties`. This map is
+     * used in `propertyNameForAttribute` which locks property names and
+     * attribute names together using this data.
      *
-     * @param {PropertiesClassConstructor} ctor PropertiesClass constructor
-     * @param {string} nane Name of attribute
-     * @return {string} Name of property.
+     * @private
+     * @param {PropertiesClassConstructor} constructor PropertiesClass constructor
+     * @return {Object} Object mapping property names to attributes.
      */
-    function propertyNameForAttributeMap(ctor) {
-      if (!ctor.hasOwnProperty(
-        JSCompiler_renameProperty('__propertyNameForAttributeMap', ctor))) {
-        ctor.__propertyNameForAttributeMap = {};
+    function propertyNameForAttributeMap(constructor) {
+      if (!constructor.hasOwnProperty(JSCompiler_renameProperty(
+        '__propertyNameForAttributeMap', constructor))) {
+        const map = constructor.__propertyNameForAttributeMap = {};
+        const props = constructor._properties;
+        for (let prop in props) {
+          const attr = constructor.attributeNameForProperty(prop);
+          map[attr] = prop;
+        }
       }
-      return ctor.__propertyNameForAttributeMap;
+      return constructor.__propertyNameForAttributeMap;
     }
 
     /**
@@ -126,7 +134,7 @@
       static get observedAttributes() {
         const props = this._properties;
         return props ? Object.keys(props).map(p => {
-          return this.prototype._attributeNameForProperty(p);
+          return this.attributeNameForProperty(p);
         }) : [];
       }
 
@@ -180,16 +188,16 @@
       }
 
       /**
-       * Overrides PropertiesChanged implementation to provide caching.
+       * Overrides PropertiesChanged implementation to discover the property
+       * associated with an attribute by revers mapping the property names in
+       * the `properties` object to the attribute name generated via
+       * `attributeNameForProperty`.
+       *
        * @param {string} name Name of property
        * @return {string} Name of attribute
        */
-      _propertyNameForAttribute(name) {
-        const cache = propertyNameForAttributeMap(this.constructor);
-        if (!cache[name]) {
-          cache[name] = super._propertyNameForAttribute(name);
-        }
-        return cache[name];
+      static propertyNameForAttribute(name) {
+        return propertyNameForAttributeMap(this)[name];
       }
 
       /**
@@ -200,8 +208,8 @@
        *
        * @protected
        */
-      _typeForProperty(name) {
-        const info = this.constructor._properties[name];
+      static typeForProperty(name) {
+        const info = this._properties[name];
         return info && info.type;
       }
 

diff --git a/lib/mixins/property-accessors.html b/lib/mixins/property-accessors.html
@@ -133,7 +133,7 @@
        *
        * @protected
        */
-      _propertyNameForAttribute(attribute) {
+      static propertyNameForAttribute(attribute) {
         return caseMap.dashToCamelCase(attribute);
       }
 
@@ -145,7 +145,7 @@
        *
        * @protected
        */
-      _attributeNameForProperty(property) {
+      static attributeNameForProperty(property) {
         return caseMap.camelToDashCase(property);
       }
 

diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html
@@ -2064,7 +2064,7 @@
        * @protected
        */
       _createReflectedProperty(property) {
-        let attr = this._attributeNameForProperty(property);
+        let attr = this.constructor.attributeNameForProperty(property);
         if (attr[0] === '-') {
           console.warn('Property ' + property + ' cannot be reflected to attribute ' +
             attr + ' because "-" is not a valid starting attribute name. Use a lowercase first letter for the property instead.');

diff --git a/test/unit/polymer.properties-element.html b/test/unit/polymer.properties-element.html
@@ -26,7 +26,8 @@
           return {
             prop: String,
             noStomp: String,
-            id: String
+            id: String,
+            camelCase: String
           };
         }
 
@@ -196,7 +197,7 @@
 
   <test-fixture id="my-element-attr">
     <template>
-      <my-element prop="attr" foo="foo"></my-element>
+      <my-element prop="attr" camelCase="camelCase"></my-element>
     </template>
   </test-fixture>
 
@@ -262,7 +263,8 @@
     test('attributes', function() {
       const fixtureEl = fixture('my-element-attr');
       assert.equal(fixtureEl.prop, 'attr');
-      assert.equal(fixtureEl._callAttributeChangedCallback, 1);
+      assert.equal(fixtureEl.camelCase, 'camelCase');
+      assert.equal(fixtureEl._callAttributeChangedCallback, 2);
       fixtureEl.removeAttribute('prop');
       assert.equal(fixtureEl.prop, null);
     });