Skip to content

Support ES6 getters and setters for data binding #4815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Support ES6 getters and setters for data binding #4815

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4443c6f
Support ES6 getters and setters for data binding
ChadKillingsworth Aug 23, 2017
1b17b88
Since the PropertyAccessors version of invalidateProperties is overri…
ChadKillingsworth Aug 26, 2017
a7e0367
Fix closure-compiler type errors from a newer build of the compiler.
ChadKillingsworth Aug 26, 2017
2d9b751
Rename the internal property in the getters/setters test to make it v…
ChadKillingsworth Sep 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion externs/closure-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,22 @@ Polymer_PropertyAccessors.prototype.__dataOld;
/** @type {Object} */
Polymer_PropertyAccessors.prototype.__dataProto;

/** @type {Object} */
/** @type {Object<string, number>} */
Polymer_PropertyAccessors.prototype.__dataHasAccessor;

/** @type {Object} */
Polymer_PropertyAccessors.prototype.__dataInstanceProps;

/**
* @const
* @type {{
* NONE: number,
* SYNTHETIC: number,
* PRE_DEFINED: number
* }}
*/
Polymer_PropertyAccessors.prototype.ACCESSOR_TYPE;

/**
* @param {string} name Name of attribute that changed
* @param {?string} old Old attribute value
Expand Down Expand Up @@ -317,6 +327,19 @@ Polymer_PropertyEffects.prototype.__readOnly;
/** @type {!TemplateInfo} */
Polymer_PropertyEffects.prototype.__templateInfo;

/**
* @const
* @type {{
* COMPUTE: string,
* REFLECT: string,
* NOTIFY: string,
* PROPAGATE: string,
* OBSERVE: string,
* READ_ONLY: string
* }}
*/
Polymer_PropertyEffects.prototype.PROPERTY_EFFECT_TYPES;

/**
* @override
* @param {!HTMLTemplateElement} template Template to stamp
Expand Down
64 changes: 53 additions & 11 deletions lib/mixins/property-accessors.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,13 @@
}
}

/** @enum */
const AccessorType = {
NONE: 0,
SYNTHETIC: 1,
PRE_DEFINED: 2
};

/**
* Element class mixin that provides basic meta-programming for creating one
* or more property accessors (getter/setter pair) that enqueue an async
Expand Down Expand Up @@ -138,13 +145,17 @@
this.__dataOld;
/** @type {Object} */
this.__dataProto;
/** @type {Object} */
/** @type {Object<string, number>} */
this.__dataHasAccessor;
/** @type {Object} */
this.__dataInstanceProps;
this._initializeProperties();
}

get ACCESSOR_TYPE() {
return AccessorType;
}

/**
* Implements native Custom Elements `attributeChangedCallback` to
* set an attribute value to a property via `_attributeToProperty`.
Expand Down Expand Up @@ -414,20 +425,50 @@
this.__dataHasAccessor = Object.assign({}, this.__dataHasAccessor);
}
if (!this.__dataHasAccessor[property]) {
this.__dataHasAccessor[property] = true;
this.__dataHasAccessor[property] = AccessorType.SYNTHETIC;
saveAccessorValue(this, property);
Object.defineProperty(this, property, {
/* eslint-disable valid-jsdoc */

const existingDescriptor = Object.getOwnPropertyDescriptor(this, property) || {};

/** @type {Object} */
const propertyDefinition = {
enumerable: existingDescriptor.enumerable || false
};

// If the property was pre-defined with an es6 getter,
// we just use that and don't need to define our own getter
if (existingDescriptor.get) {
propertyDefinition.get = existingDescriptor.get;
this.__dataHasAccessor[property] = AccessorType.PRE_DEFINED;
} else {
// eslint-disable-next-line valid-jsdoc
/** @this {PropertyAccessors} */
get: function() {
propertyDefinition.get = function() {
return this.__data[property];
},
};
}

// If the property was pre-defined with an es6 setter,
// use that, but also call our own _setProperty function
// so that change notifications are propagated
if (existingDescriptor.set) {
const existingSetter = /** @type {{set: function(*)}} */ (existingDescriptor).set.bind(this);
// eslint-disable-next-line valid-jsdoc
/** @this {PropertyAccessors} */
set: readOnly ? function() {} : function(value) {
propertyDefinition.set = function(value) {
// Call the predefined setter
existingSetter(value);
// Invalidate properties if changed - includes dirty checking
this._setProperty(property, value);
}
/* eslint-enable */
});
};
} else if (this.__dataHasAccessor[property] !== AccessorType.PRE_DEFINED) {
// eslint-disable-next-line valid-jsdoc
propertyDefinition.set = readOnly ? function() {} : /** @this {PropertyAccessors} */ function(value) {
this._setProperty(property, value);
};
}

Object.defineProperty(this, property, propertyDefinition);
}
}

Expand All @@ -438,7 +479,8 @@
* @return {boolean} True if an accessor was created
*/
_hasAccessor(property) {
return this.__dataHasAccessor && this.__dataHasAccessor[property];
return this.__dataHasAccessor !== undefined &&
(this.__dataHasAccessor[property] || AccessorType.NONE) !== AccessorType.NONE;
}

/**
Expand Down
11 changes: 10 additions & 1 deletion lib/mixins/property-effects.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1359,7 +1359,7 @@
return true;
}
} else {
if (this.__dataHasAccessor && this.__dataHasAccessor[path]) {
if (this.__dataHasAccessor && this.__dataHasAccessor[/** @type {string} */ (path)]) {
return this._setPendingProperty(/**@type{string}*/(path), value, shouldNotify);
} else {
this[path] = value;
Expand Down Expand Up @@ -2260,6 +2260,15 @@
if (!wasPreBound) {
for (let prop in templateInfo.propertyEffects) {
this._createPropertyAccessor(prop);

// For any predefined getters without setters, add a read only effect
// so that change notifications don't attempt to update the data cache
if (this.ACCESSOR_TYPE && this.__dataHasAccessor[prop] === this.ACCESSOR_TYPE.PRE_DEFINED) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this code here instead of in _createPropertyAccessor, any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking was since this was adding a property effect - it shouldn't be in property accessors. I was trying to consider the cases where PropertyAccessors was mixed in without mixing in PropertyEffects.

const descriptor = Object.getOwnPropertyDescriptor(this, prop);
if (descriptor && !descriptor.set) {
this._createReadOnlyProperty(prop, false);
}
}
}
}
if (instanceBinding) {
Expand Down
38 changes: 36 additions & 2 deletions test/unit/property-accessors.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@
<link rel="import" href="../../polymer.html">
<body>

<x-foo prop1="a" prop2="b"></x-foo>
<x-foo prop1="a" prop2="b" prop3="c"></x-foo>

<script>
HTMLImports.whenReady(function() {
class XFoo extends Polymer.PropertyAccessors(HTMLElement) {
static get observedAttributes() {
return ['prop1', 'prop2'];
return ['prop1', 'prop2', 'prop3'];
}
get prop3() { return this.prop3_; }
set prop3(value) { this.prop3_ = value; }
get prop4() { return 'prop4' + (new Date()).getTime(); }
constructor() {
super();
this._propertiesChanged = sinon.spy();
Expand All @@ -34,6 +37,8 @@
}
XFoo.createPropertiesForAttributes();
window.XFoo = XFoo;
window.XFooProp3Descriptor = Object.getOwnPropertyDescriptor(XFoo.prototype, 'prop3');
window.XFooProp4Descriptor = Object.getOwnPropertyDescriptor(XFoo.prototype, 'prop4');
customElements.define('x-foo', XFoo);
});
</script>
Expand All @@ -47,33 +52,54 @@
assert.ok(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop2'));
});

test('createPropertiesForAttributes preserves existing getters', function() {
assert.ok(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop3')
.get.toString().indexOf('() { return this.prop3_; }') > 0);
assert.ok(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop4')
.get.toString().indexOf("() { return 'prop4' + (new Date()).getTime(); }") > 0);
});

test('createPropertiesForAttributes properly handles class setters', function() {
assert.ok(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop3').set);
assert.ok(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop3')
.set.toString().indexOf('(value) { this.prop3_ = value; }') < 0);
assert.equal(Object.getOwnPropertyDescriptor(window.XFoo.prototype, 'prop4').set, undefined);
});

test('attributes reflected to properties via upgrade', function() {
var el = document.querySelector('x-foo');
assert.equal(el.prop1, 'a');
assert.equal(el.prop2, 'b');
assert.equal(el.prop3, 'c');
});

test('setting properties results in _propertiesChanged', function(done) {
var el = document.createElement('x-foo');
document.body.appendChild(el);
el.prop1 = 'a';
el.prop2 = 'b';
el.prop3 = 'c';
assert.equal(el._propertiesChanged.callCount, 0, '_propertiesChanged is not async');
setTimeout(function() {
assert.isTrue(el._propertiesChanged.calledOnce);
assert.equal(el._propertiesChanged.getCall(0).args[0].prop1, 'a');
assert.equal(el._propertiesChanged.getCall(0).args[0].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(0).args[0].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop1, 'a');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(0).args[2].prop1, undefined);
assert.equal(el._propertiesChanged.getCall(0).args[2].prop2, undefined);
assert.equal(el._propertiesChanged.getCall(0).args[2].prop3, undefined);
assert.equal(el.prop1, 'a');
assert.equal(el.prop2, 'b');
assert.equal(el.prop3, 'c');
el.prop1 = 'aa';
setTimeout(function() {
assert.isTrue(el._propertiesChanged.calledTwice);
assert.equal(el._propertiesChanged.getCall(1).args[0].prop1, 'aa');
assert.equal(el._propertiesChanged.getCall(1).args[0].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(1).args[0].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(1).args[1].prop1, 'aa');
assert.equal(el._propertiesChanged.getCall(1).args[2].prop1, 'a');
assert.isFalse('prop2' in el._propertiesChanged.getCall(1).args[1]);
Expand All @@ -88,25 +114,33 @@
document.body.appendChild(el);
el.setAttribute('prop1', 'a');
el.setAttribute('prop2', 'b');
el.setAttribute('prop3', 'c');
setTimeout(function() {
assert.isTrue(el._propertiesChanged.calledOnce);
assert.equal(el._propertiesChanged.getCall(0).args[0].prop1, 'a');
assert.equal(el._propertiesChanged.getCall(0).args[0].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(0).args[0].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop1, 'a');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(0).args[1].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(0).args[2].prop1, undefined);
assert.equal(el._propertiesChanged.getCall(0).args[2].prop2, undefined);
assert.equal(el._propertiesChanged.getCall(0).args[2].prop3, undefined);
assert.equal(el.prop1, 'a');
assert.equal(el.prop2, 'b');
assert.equal(el.prop3, 'c');
el.setAttribute('prop1', 'aa');
setTimeout(function() {
assert.isTrue(el._propertiesChanged.calledTwice);
assert.equal(el._propertiesChanged.getCall(1).args[0].prop1, 'aa');
assert.equal(el._propertiesChanged.getCall(1).args[0].prop2, 'b');
assert.equal(el._propertiesChanged.getCall(1).args[0].prop3, 'c');
assert.equal(el._propertiesChanged.getCall(1).args[1].prop1, 'aa');
assert.equal(el._propertiesChanged.getCall(1).args[2].prop1, 'a');
assert.isFalse('prop2' in el._propertiesChanged.getCall(1).args[1]);
assert.isFalse('prop2' in el._propertiesChanged.getCall(1).args[2]);
assert.isFalse('prop3' in el._propertiesChanged.getCall(1).args[1]);
assert.isFalse('prop3' in el._propertiesChanged.getCall(1).args[2]);
done();
});

Expand Down
24 changes: 23 additions & 1 deletion test/unit/property-effects-elements.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,4 +1025,26 @@
this.xChanged = sinon.spy();
}
});
</script>
</script>
<dom-module id="x-getters-setters">
<template>
<div id="prop1">[[prop1]]</div>
<div id="prop2">[[prop2]]</div>
<div id="addProps">[[addProps(prop1, prop2)]]</div>
<input id="prop1TwoWay" value="{{prop1::input}}"/>
<input id="prop2TwoWay" value="{{prop2::input}}"/>
</template>
<script>
class XGettersSetters extends Polymer.Element {
static get is() { return 'x-getters-setters'; }
get prop1() { return this.prop1_; }
set prop1(value) { this.prop1_ = value; }
get prop2() { return 'prop2-' + (new Date()).getTime(); }

addProps(prop1, prop2) {
return `${prop1} ${prop2}`;
}
}
customElements.define(XGettersSetters.is, XGettersSetters);
</script>
</dom-module>
45 changes: 45 additions & 0 deletions test/unit/property-effects.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1760,6 +1760,51 @@

});

suite('ES6 getters/setters', function() {
test('notify path bypasses internal cache to update template', function(done) {
let el = document.createElement('x-getters-setters');
el.prop1 = 'prop1';
document.body.appendChild(el);
setTimeout(() => {
let originalProp2 = el.$.prop2.innerText;
assert.equal(el.$.prop1.innerText, 'prop1');
assert.equal(originalProp2.indexOf('prop2'), 0);
assert.equal(el.$.addProps.innerText.indexOf('prop1 prop2'), 0);
assert.equal(el.$.prop1TwoWay.value, 'prop1');
let originalProp2TwoWay = el.$.prop2TwoWay.value;
assert.equal(originalProp2TwoWay.indexOf('prop2'), 0);

el.prop1_ = 'prop1changed';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like to see a test that sets both prop1 and prop2 and asserts that el.$.addProps is set correctly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and initially I missed the fact that this is prop1_. Maybe rename this variable to make it clearer that this is the internal property cache, to make it more distinguishable. Something like _internal_prop1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prop2 has a getter but no setter. It's read only. Do you want me to add a 3rd property to test that?

I initially had that but removed it. Since PropertyEffects overrides PropertyAccessor to make _propertiesChanged synchronous, I removed the 3rd property.

el.notifyPath('prop1');
el.notifyPath('prop2');
setTimeout(() => {
assert.equal(el.$.prop1.innerText, 'prop1changed');
assert.equal(el.$.addProps.innerText.indexOf('prop1changed prop2'), 0);
assert.equal(el.$.prop1TwoWay.value, 'prop1changed');
assert.notEqual(el.$.prop2.innerText, originalProp2);
assert.notEqual(el.$.prop2TwoWay.value, originalProp2TwoWay);
done();
});
});
});

test('setters update template', function(done) {
let el = document.createElement('x-getters-setters');
el.prop1 = 'prop1';
document.body.appendChild(el);
setTimeout(() => {
assert.equal(el.$.prop1.innerText, 'prop1');
assert.equal(el.$.addProps.innerText.indexOf('prop1 prop2'), 0);

el.prop1 = 'prop1changed';
setTimeout(() => {
assert.equal(el.$.prop1.innerText, 'prop1changed');
assert.equal(el.$.addProps.innerText.indexOf('prop1changed prop2'), 0);
done();
});
});
});
});
</script>

</body>
Expand Down