Skip to content

Commit

Permalink
Address review feedback
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
Steven Orvell committed Dec 19, 2019
1 parent 7b0c57a commit f8dfaa5
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 39 deletions.
34 changes: 12 additions & 22 deletions lib/legacy/class.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.]
Expand Down Expand Up @@ -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 = {};
Expand Down Expand Up @@ -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() {
Expand All @@ -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 */
Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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 */
Expand All @@ -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`.
Expand Down Expand Up @@ -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
Expand Down
22 changes: 11 additions & 11 deletions lib/mixins/disable-upgrade-mixin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
19 changes: 13 additions & 6 deletions test/unit/legacy-noattributes.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -43,12 +44,12 @@

<dom-module id="x-attrs">
<template>
<x-child id="child1" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]"></x-child>
<x-child id="child2" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" disable-upgrade></x-child>
<x-child id="child3" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" disable-upgrade$="[[disabled]]"></x-child>
<x-child id="child1" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" camel-case="camelCase"></x-child>
<x-child id="child2" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" camel-case="[[camelCase]]" disable-upgrade></x-child>
<x-child id="child3" foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" camel-case="[[camelCase]]" disable-upgrade$="[[disabled]]"></x-child>
<div id="ifContainer">
<dom-if if="[[shouldIf]]">
<template><x-child foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]"></x-child></template>
<template><x-child foo="x-attrs.foo" bar$="[[bar]]" zot$="[[zot]]" camel-case="[[camelCase]]"></x-child></template>
</dom-if>
</div>
</template>
Expand All @@ -61,6 +62,7 @@
bar: {type: String, value: 'bar'},
zot: Boolean,
shouldIf: Boolean,
camelCase: String,
disabled: {type: Boolean, value: 'true'}
}
});
Expand All @@ -69,7 +71,7 @@

<test-fixture id="declarative">
<template>
<x-attrs id="configured" foo="foo" bar="bar"></x-attrs>
<x-attrs id="configured" foo="foo" bar="bar" camel-case="camelCase"></x-attrs>
</template>
</test-fixture>

Expand All @@ -89,13 +91,15 @@
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', () => {
assert.equal(el.$.child1.getAttribute('bar'), 'bar');
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', () => {
Expand All @@ -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');
Expand All @@ -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');
});


Expand Down

0 comments on commit f8dfaa5

Please sign in to comment.