Skip to content
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.

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

Support ES6 getters and setters for data binding #4815

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
66 changes: 54 additions & 12 deletions lib/mixins/property-accessors.html
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 @@ -413,21 +424,51 @@
if (!this.hasOwnProperty('__dataHasAccessor')) {
this.__dataHasAccessor = Object.assign({}, this.__dataHasAccessor);
}
if (!this.__dataHasAccessor[property]) {
this.__dataHasAccessor[property] = true;
if ((this.__dataHasAccessor[property] || AccessorType.NONE) === AccessorType.NONE) {
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
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
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
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.internal_prop1_; }
set prop1(value) { this.internal_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
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.internal_prop1_ = 'prop1changed';
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