Merge pull request #5474 from Polymer/legacy-warnings-3.x

Legacy warnings 3.x

Author: kevinpschaaf

lib/mixins/element-mixin.js

@@ -195,7 +195,6 @@ export const ElementMixin = dedupingMixin(base => {
    * disables the effect, the setter would fail unexpectedly.
    * Based on feedback, we may want to try to make effects more malleable
    * and/or provide an advanced api for manipulating them.
-   * Also consider adding warnings when an effect cannot be changed.
    *
    * @param {!PolymerElement} proto Element class prototype to add accessors
    *   and effects to
@@ -217,17 +216,27 @@ export const ElementMixin = dedupingMixin(base => {
     // setup where multiple triggers for setting a property)
     // While we do have `hasComputedEffect` this is set on the property's
     // dependencies rather than itself.
-    if (info.computed && !proto._hasReadOnlyEffect(name)) {
-      proto._createComputedProperty(name, info.computed, allProps);
+    if (info.computed) {
+      if (proto._hasReadOnlyEffect(name)) {
+        console.warn(`Cannot redefine computed property '${name}'.`);
+      } else {
+        proto._createComputedProperty(name, info.computed, allProps);
+      }
     }
     if (info.readOnly && !proto._hasReadOnlyEffect(name)) {
       proto._createReadOnlyProperty(name, !info.computed);
+    } else if (info.readOnly === false && proto._hasReadOnlyEffect(name)) {
+      console.warn(`Cannot make readOnly property '${name}' non-readOnly.`);
     }
     if (info.reflectToAttribute && !proto._hasReflectEffect(name)) {
       proto._createReflectedProperty(name);
+    } else if (info.reflectToAttribute === false && proto._hasReflectEffect(name)) {
+      console.warn(`Cannot make reflected property '${name}' non-reflected.`);
     }
     if (info.notify && !proto._hasNotifyEffect(name)) {
       proto._createNotifyingProperty(name);
+    } else if (info.notify === false && proto._hasNotifyEffect(name)) {
+      console.warn(`Cannot make notify property '${name}' non-notify.`);
     }
     // always add observer
     if (info.observer) {
@@ -718,7 +727,7 @@ export const ElementMixin = dedupingMixin(base => {
     }
 
     /**
-     * Overrides `PropertyAccessors` to add map of dynamic functions on
+     * Overrides `PropertyEffects` to add map of dynamic functions on
      * template info, for consumption by `PropertyEffects` template binding
      * code. This map determines which method templates should have accessors
      * created for them.
@@ -734,6 +743,32 @@ export const ElementMixin = dedupingMixin(base => {
       return super._parseTemplateContent(template, templateInfo, nodeInfo);
     }
 
+    /**
+     * Overrides `PropertyEffects` to warn on use of undeclared properties in
+     * template.
+     *
+     * @param {Object} templateInfo Template metadata to add effect to
+     * @param {string} prop Property that should trigger the effect
+     * @param {Object=} effect Effect metadata object
+     * @return {void}
+     * @protected
+     */
+    static _addTemplatePropertyEffect(templateInfo, prop, effect) {
+      // Warn if properties are used in template without being declared.
+      // Properties must be listed in `properties` to be included in
+      // `observedAttributes` since CE V1 reads that at registration time, and
+      // since we want to keep template parsing lazy, we can't automatically
+      // add undeclared properties used in templates to `observedAttributes`.
+      // The warning is only enabled in `legacyOptimizations` mode, since
+      // we don't want to spam existing users who might have adopted the
+      // shorthand when attribute deserialization is not important.
+      if (legacyOptimizations && !(prop in this._properties)) {
+        console.warn(`Property '${prop}' used in template but not declared in 'properties'; ` +
+          `attribute will not be observed.`);
+      }
+      return super._addTemplatePropertyEffect(templateInfo, prop, effect);
+    }
+
   }
 
   return PolymerElement;

test/unit/property-effects.html

@@ -25,8 +25,8 @@
 
 <script type="module">
 import './property-effects-elements.js';
-import { Polymer } from '../../lib/legacy/polymer-fn.js';
-import { setSanitizeDOMValue, sanitizeDOMValue } from '../../lib/utils/settings.js';
+import { Polymer, html } from '../../polymer-legacy.js';
+import { setSanitizeDOMValue, sanitizeDOMValue, setLegacyOptimizations } from '../../lib/utils/settings.js';
 import { PropertyEffects } from '../../lib/mixins/property-effects.js';
 
 suite('single-element binding effects', function() {
@@ -1872,6 +1872,110 @@
     document.body.removeChild(el);
   });
 
+});
+
+suite('warn on legacy differences', () => {
+
+  setup(function() {
+    sinon.spy(console, 'warn');
+  });
+
+  teardown(function() {
+    setLegacyOptimizations(false);
+    console.warn.restore();
+  });
+
+  test('warn if non-declared property used in binding', () => {
+    setLegacyOptimizations(true);
+    Polymer({
+      is: 'x-warn-undeclared-binding',
+      _template: html`
+        <div attr$="[[undeclaredToAttr]]"
+             prop="[[undeclaredToProp]]">
+             [[undeclaredToText]]</div>`
+    });
+    const el = document.createElement('x-warn-undeclared-binding');
+    document.body.appendChild(el);
+    document.body.removeChild(el);
+    assert.equal(console.warn.callCount, 3);
+  });
+
+  test('warn when re-declaring a computed property', () => {
+    Polymer({
+      is: 'x-warn-redeclared-computed',
+      behaviors: [{
+        properties: {
+          a: { computed: 'compute(x)' }
+        }
+      }, {
+        properties: {
+          a: { computed: 'compute(y)' }
+        }
+      }]
+    });
+    const el = document.createElement('x-warn-redeclared-computed');
+    document.body.appendChild(el);
+    document.body.removeChild(el);
+    assert.equal(console.warn.callCount, 1);
+  });
+
+  test('warn when disabling readOnly on a readOnly property', () => {
+    Polymer({
+      is: 'x-warn-disable-readonly',
+      behaviors: [{
+        properties: {
+          a: { readOnly: true }
+        }
+      }, {
+        properties: {
+          a: { readOnly: false }
+        }
+      }]
+    });
+    const el = document.createElement('x-warn-disable-readonly');
+    document.body.appendChild(el);
+    document.body.removeChild(el);
+    assert.equal(console.warn.callCount, 1);
+  });
+
+  test('warn when disabling reflect on a reflect property', () => {
+    Polymer({
+      is: 'x-warn-disable-reflect',
+      behaviors: [{
+        properties: {
+          a: { reflectToAttribute: true }
+        }
+      }, {
+        properties: {
+          a: { reflectToAttribute: false }
+        }
+      }]
+    });
+    const el = document.createElement('x-warn-disable-reflect');
+    document.body.appendChild(el);
+    document.body.removeChild(el);
+    assert.equal(console.warn.callCount, 1);
+  });
+
+  test('warn when disabling notify on a notify property', () => {
+    Polymer({
+      is: 'x-warn-disable-notify',
+      behaviors: [{
+        properties: {
+          a: { notify: true }
+        }
+      }, {
+        properties: {
+          a: { notify: false }
+        }
+      }]
+    });
+    const el = document.createElement('x-warn-disable-notify');
+    document.body.appendChild(el);
+    document.body.removeChild(el);
+    assert.equal(console.warn.callCount, 1);
+  });
+
 });
 </script>