Skip to content

Commit

Permalink
Fix issue with defaults overriding bound values when disable-upgrade …
Browse files Browse the repository at this point in the history
…is used.
  • Loading branch information
Steven Orvell committed Jul 26, 2019
1 parent 6914078 commit cd6d5d0
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 10 deletions.
11 changes: 11 additions & 0 deletions lib/mixins/disable-upgrade-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ export const DisableUpgradeMixin = dedupingMixin((base) => {
}
}

// 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
// `_initializeProperties` which is called after initial properties
// have been set when the element starts upgrade-disabled.
/** @override */
_canApplyPropertyDefault(property) {
return super._canApplyPropertyDefault(property) &&
!(this.__isUpgradeDisabled && this._isPropertyPending(property));
}

/**
* @override
* @param {string} name Attribute name.
Expand Down
16 changes: 12 additions & 4 deletions lib/mixins/element-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -562,10 +562,7 @@ export const ElementMixin = dedupingMixin(base => {
}
for (let p in p$) {
let info = p$[p];
// Don't set default value if there is already an own property, which
// happens when a `properties` property with default but no effects had
// a property set (e.g. bound) by its host before upgrade
if (!this.hasOwnProperty(p)) {
if (this._canApplyPropertyDefault(p)) {
let value = typeof info.value == 'function' ?
info.value.call(this) :
info.value;
Expand All @@ -580,6 +577,17 @@ export const ElementMixin = dedupingMixin(base => {
}
}

/**
* @param {string} property
* @returns {boolean} Returns true if the property default can be applied.
*/
// A property default can be overridden when a `properties` property with
// default but no effects had a property set (e.g. bound) by its host
// before upgrade.
_canApplyPropertyDefault(property) {
return !this.hasOwnProperty(property);
}

/**
* Gather style text for a style element in the template.
*
Expand Down
8 changes: 8 additions & 0 deletions lib/mixins/properties-changed.js
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,14 @@ export const PropertiesChanged = dedupingMixin(
}
/* eslint-enable */

/**
* @param {string} property
* @return {boolean} Returns true if the property is pending.
*/
_isPropertyPending(property) {
return !!(this.__dataPending && this.__dataPending.hasOwnProperty(property));
}

/**
* Marks the properties as invalid, and enqueues an async
* `_propertiesChanged` callback.
Expand Down
34 changes: 28 additions & 6 deletions test/unit/disable-upgrade.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
}
</style>
<h2 id="element">[[prop]]</h2>
<!-- force an accessor for the default property -->
default: [[default]]
</template>

<script type="module">
Expand All @@ -41,6 +43,10 @@ <h2 id="element">[[prop]]</h2>
return {
prop: {
type: String
},
default: {
type: String,
value: 'default'
}
};
}
Expand Down Expand Up @@ -81,6 +87,8 @@ <h2 id="element">[[prop]]</h2>
}
</style>
<h2 id="element">[[prop]]</h2>
<!-- force an accessor for the default property -->
default: [[default]]
</template>

<script type="module">
Expand All @@ -95,6 +103,10 @@ <h2 id="element">[[prop]]</h2>
properties: {
prop: {
type: String
},
default: {
type: String,
value: 'default'
}
},
listeners: {
Expand Down Expand Up @@ -137,8 +149,8 @@ <h2 id="element">[[prop]]</h2>

<template>
<x-disabled id="enabledEl">Disabled</x-disabled>
<x-disabled id="disabledEl" disable-upgrade>Disabled</x-disabled>
<x-disabled id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled>
<x-disabled id="disabledEl" disable-upgrade default="custom">Disabled</x-disabled>
<x-disabled id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]" default="[[default]]">Disabled</x-disabled>
</template>

<script type="module">
Expand All @@ -148,7 +160,8 @@ <h2 id="element">[[prop]]</h2>
static get is() { return 'my-element'; }
static get properties() {
return {
upgradeDisabled: {value: true}
upgradeDisabled: {value: true},
default: {type: String, value: 'custom'}
};
}
enable() {
Expand All @@ -165,8 +178,8 @@ <h2 id="element">[[prop]]</h2>

<template>
<x-disabled-legacy id="enabledEl">Disabled</x-disabled-legacy>
<x-disabled-legacy id="disabledEl" disable-upgrade>Disabled</x-disabled-legacy>
<x-disabled-legacy id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled-legacy>
<x-disabled-legacy id="disabledEl" disable-upgrade default="custom">Disabled</x-disabled-legacy>
<x-disabled-legacy id="disabledBoundEl" disable-upgrade$="[[upgradeDisabled]]" default="[[default]]">Disabled</x-disabled-legacy>
<x-disabled-legacy-reg1 id="disabledRegEl" disable-upgrade>Disabled</x-disabled-legacy-reg1>
<x-disabled-legacy-reg2 id="disabledRegBoundEl" disable-upgrade$="[[upgradeDisabled]]">Disabled</x-disabled-legacy-reg2>

Expand All @@ -177,7 +190,8 @@ <h2 id="element">[[prop]]</h2>
Polymer({
is: 'my-element-legacy',
properties: {
upgradeDisabled: { type: Boolean, value: true }
upgradeDisabled: { type: Boolean, value: true },
default: {type: String, value: 'custom'}
},
enable() {
this.$.disabledEl.removeAttribute('disable-upgrade');
Expand Down Expand Up @@ -281,8 +295,10 @@ <h2 id="element">[[prop]]</h2>
assert.equal(el.$.enabledEl.$.element.textContent, 'enabled!');
assert.notOk(el.$.disabledEl.wasConnected);
assert.notOk(el.$.disabledEl.enabled);
assert.equal(el.$.disabledEl.default, 'custom');
assert.notOk(el.$.disabledEl.$);
assert.notOk(el.$.disabledBoundEl.enabled);
assert.equal(el.$.disabledBoundEl.default, 'custom');
assert.notOk(el.$.disabledBoundEl.wasConnected);
assert.notOk(el.$.disabledBoundEl.$);
});
Expand All @@ -298,8 +314,10 @@ <h2 id="element">[[prop]]</h2>
assert.ok(el.$.disabledEl.enabled);
assert.ok(el.$.disabledEl.$.element);
assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
assert.equal(el.$.disabledEl.default, 'custom');
assert.ok(el.$.disabledEl.wasConnected);
assert.ok(el.$.disabledBoundEl.enabled);
assert.equal(el.$.disabledBoundEl.default, 'custom');
assert.ok(el.$.disabledBoundEl.$.element);
assert.ok(el.$.disabledBoundEl.wasConnected);
assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');
Expand Down Expand Up @@ -358,13 +376,15 @@ <h2 id="element">[[prop]]</h2>
assert.equal(el.$.enabledEl.fooHandler.callCount, 1);
assert.notOk(el.$.disabledEl.hasCreated);
assert.notOk(el.$.disabledEl.enabled);
assert.equal(el.$.disabledEl.default, 'custom');
assert.notOk(el.$.disabledEl.$);
assert.notOk(el.$.disabledEl.wasConnected);
el.$.disabledEl.fooHandler = sinon.spy();
el.$.disabledEl.fire('foo');
assert.equal(el.$.disabledEl.fooHandler.callCount, 0);
assert.notOk(el.$.disabledBoundEl.hasCreated);
assert.notOk(el.$.disabledBoundEl.enabled);
assert.equal(el.$.disabledBoundEl.default, 'custom');
assert.notOk(el.$.disabledBoundEl.$);
assert.notOk(el.$.disabledBoundEl.wasConnected);
el.$.disabledBoundEl.fooHandler = sinon.spy();
Expand All @@ -376,6 +396,7 @@ <h2 id="element">[[prop]]</h2>
el.enable();
assert.ok(el.$.disabledEl.hasCreated);
assert.ok(el.$.disabledEl.enabled);
assert.equal(el.$.disabledEl.default, 'custom');
assert.ok(el.$.disabledEl.$.element);
assert.ok(el.$.disabledEl.wasConnected);
assert.equal(el.$.disabledEl.$.element.textContent, 'enabled!');
Expand All @@ -384,6 +405,7 @@ <h2 id="element">[[prop]]</h2>
assert.equal(el.$.disabledEl.fooHandler.callCount, 1);
assert.ok(el.$.disabledBoundEl.hasCreated);
assert.ok(el.$.disabledBoundEl.enabled);
assert.equal(el.$.disabledBoundEl.default, 'custom');
assert.ok(el.$.disabledBoundEl.$.element);
assert.ok(el.$.disabledBoundEl.wasConnected);
assert.equal(el.$.disabledBoundEl.$.element.textContent, 'enabled!');
Expand Down

0 comments on commit cd6d5d0

Please sign in to comment.