From f8dfaa5628f49f4bcbe9e0daa62a0b583b24b207 Mon Sep 17 00:00:00 2001 From: Steven Orvell Date: Thu, 19 Dec 2019 12:31:06 -0800 Subject: [PATCH] Address review feedback * DisableUpgradeMixin: fIx finding superClass observedAttributes getter so that it's always correct * Class: grab observedAttributes getter 1x off of LegacyElement * Add tests for camelCase properties used with dash cased attributes. --- lib/legacy/class.js | 34 ++++++++++------------------- lib/mixins/disable-upgrade-mixin.js | 22 +++++++++---------- test/unit/legacy-noattributes.html | 19 +++++++++++----- 3 files changed, 36 insertions(+), 39 deletions(-) diff --git a/lib/legacy/class.js b/lib/legacy/class.js index 08234ff41f..a625576cdc 100644 --- a/lib/legacy/class.js +++ b/lib/legacy/class.js @@ -10,10 +10,10 @@ subject to an additional IP rights grant found at http://polymer.github.io/PATEN import { LegacyElementMixin } from './legacy-element-mixin.js'; import { legacyOptimizations, legacyNoObservedAttributes } from '../utils/settings.js'; +import { findObservedAttributesGetter } from '../mixins/disable-upgrade-mixin.js'; import { wrap } from '../utils/wrap.js'; const DISABLED_ATTR = 'disable-upgrade'; -let observedAttributesGetter; const lifecycleProps = { attached: true, @@ -179,6 +179,9 @@ function mergeProperties(target, source) { } } +const LegacyElement = LegacyElementMixin(HTMLElement); +const observedAttributesGetter = findObservedAttributesGetter(LegacyElement); + /* Note about construction and extension of legacy classes. [Changed in Q4 2018 to optimize performance.] @@ -211,22 +214,6 @@ function mergeProperties(target, source) { */ function GenerateClassFromInfo(info, Base, behaviors) { - // Work around for closure bug #126934458. Using `super` in a property - // getter does not work so instead we search the Base prototype for an - // implementation of observedAttributes so that we can override and call - // the `super` getter. Note, this is done one time ever because we assume - // that `Base` is always comes from `Polymer.LegacyElementMixn`. - if (!observedAttributesGetter) { - let ctor = Base; - while (ctor && !observedAttributesGetter) { - const desc = Object.getOwnPropertyDescriptor(ctor, 'observedAttributes'); - if (desc) { - observedAttributesGetter = desc.get; - } - ctor = Object.getPrototypeOf(ctor.prototype).constructor; - } - } - // manages behavior and lifecycle processing (filled in after class definition) let behaviorList; const lifecycle = {}; @@ -477,20 +464,20 @@ function GenerateClassFromInfo(info, Base, behaviors) { } } - // NOTE: Below is an inlined version of DisableUpgradeMixin. It is inlined - // as a performance optimization to avoid the need to place the mixin on - // top of every legacy element. + // NOTE: Inlined for perf from version of DisableUpgradeMixin. constructor() { super(); /** @type {boolean|undefined} */ this.__isUpgradeDisabled; } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. static get observedAttributes() { return legacyNoObservedAttributes ? [] : observedAttributesGetter.call(this).concat(DISABLED_ATTR); } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. // Prevent element from initializing properties when it's upgrade disabled. /** @override */ _initializeProperties() { @@ -501,6 +488,7 @@ function GenerateClassFromInfo(info, Base, behaviors) { } } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. // Prevent element from enabling properties when it's upgrade disabled. // Normally overriding connectedCallback would be enough, but dom-* elements /** @override */ @@ -510,6 +498,7 @@ function GenerateClassFromInfo(info, Base, behaviors) { } } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. // If the element starts upgrade-disabled and a property is set for // which an accessor exists, the default should not be applied. // This additional check is needed because defaults are applied via @@ -521,6 +510,7 @@ function GenerateClassFromInfo(info, Base, behaviors) { !(this.__isUpgradeDisabled && this._isPropertyPending(property)); } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. /** * @override * @param {string} name Attribute name. @@ -546,6 +536,7 @@ function GenerateClassFromInfo(info, Base, behaviors) { } } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. // Prevent element from connecting when it's upgrade disabled. // This prevents user code in `attached` from being called. /** @override */ @@ -555,6 +546,7 @@ function GenerateClassFromInfo(info, Base, behaviors) { } } + // NOTE: Inlined for perf from version of DisableUpgradeMixin. // Prevent element from disconnecting when it's upgrade disabled. // This avoids allowing user code `detached` from being called without a // paired call to `attached`. @@ -599,8 +591,6 @@ function GenerateClassFromInfo(info, Base, behaviors) { return PolymerGenerated; } -const LegacyElement = LegacyElementMixin(HTMLElement); - /** * Generates a class that extends `LegacyElement` based on the * provided info object. Metadata objects on the `info` object diff --git a/lib/mixins/disable-upgrade-mixin.js b/lib/mixins/disable-upgrade-mixin.js index 868cfc49d2..f6d380fc2f 100644 --- a/lib/mixins/disable-upgrade-mixin.js +++ b/lib/mixins/disable-upgrade-mixin.js @@ -15,7 +15,16 @@ import { wrap } from '../utils/wrap.js'; const DISABLED_ATTR = 'disable-upgrade'; -let observedAttributesGetter; +export const findObservedAttributesGetter = (ctor) => { + while (ctor) { + const desc = Object.getOwnPropertyDescriptor(ctor, 'observedAttributes'); + if (desc) { + return desc.get; + } + ctor = Object.getPrototypeOf(ctor.prototype).constructor; + } + return () => []; +}; /** * Element class mixin that allows the element to boot up in a non-enabled @@ -58,16 +67,7 @@ export const DisableUpgradeMixin = dedupingMixin((base) => { // implementation of observedAttributes so that we can override and call // the `super` getter. Note, this is done one time ever because we assume // that `Base` is always comes from `Polymer.LegacyElementMixn`. - if (!observedAttributesGetter) { - let ctor = superClass; - while (ctor && !observedAttributesGetter) { - const desc = Object.getOwnPropertyDescriptor(ctor, 'observedAttributes'); - if (desc) { - observedAttributesGetter = desc.get; - } - ctor = Object.getPrototypeOf(ctor.prototype).constructor; - } - } + let observedAttributesGetter = findObservedAttributesGetter(superClass); /** * @polymer diff --git a/test/unit/legacy-noattributes.html b/test/unit/legacy-noattributes.html index d44a347518..1594dd735b 100644 --- a/test/unit/legacy-noattributes.html +++ b/test/unit/legacy-noattributes.html @@ -32,7 +32,8 @@ properties: { foo: {type: String, value: 'default'}, bar: {type: String, value: 'default'}, - zot: {type: Boolean, value: false, reflectToAttribute: true} + zot: {type: Boolean, value: false, reflectToAttribute: true}, + camelCase: String }, ready() { this.isEnabled = true; @@ -43,12 +44,12 @@ @@ -61,6 +62,7 @@ bar: {type: String, value: 'bar'}, zot: Boolean, shouldIf: Boolean, + camelCase: String, disabled: {type: Boolean, value: 'true'} } }); @@ -69,7 +71,7 @@ @@ -89,6 +91,7 @@ assert.equal(el.$.child1.getAttribute('foo'), 'x-attrs.foo'); assert.equal(el.$.child1.foo, "x-attrs.foo"); assert.equal(el.$.child1.$.content.textContent, 'x-attrs.foo'); + assert.equal(el.camelCase, 'camelCase'); }); test('static attribute bindings', () => { @@ -96,6 +99,7 @@ assert.equal(el.$.child1.bar, 'bar'); assert.equal(el.$.child1.getAttribute('zot'), null); assert.equal(el.$.child1.zot, false); + assert.equal(el.$.child1.camelCase, 'camelCase'); }); test('dynamic attribute bindings', () => { @@ -114,6 +118,7 @@ assert.equal(child.getAttribute('foo'), 'x-attrs.foo'); assert.equal(child.foo, "x-attrs.foo"); assert.equal(child.$.content.textContent, 'x-attrs.foo'); + assert.equal(child.camelCase, 'camelCase'); // assert.equal(child.getAttribute('bar'), 'bar'); assert.equal(child.bar, 'bar'); @@ -132,12 +137,14 @@ assert.notOk(el.$.child2.isEnabled); el.$.child2.removeAttribute('disable-upgrade'); assert.isTrue(el.$.child2.isEnabled); + assert.equal(el.$.child2.camelCase, 'camelCase'); }); test('disable-upgrade binding', () => { assert.notOk(el.$.child3.isEnabled); el.disabled = false; assert.isTrue(el.$.child3.isEnabled); + assert.equal(el.$.child3.camelCase, 'camelCase'); });