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.

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

Use stronger check for PropertyEffects clients. Fixes #5107 #5108

Merged
merged 5 commits into from
Feb 21, 2018
Merged

Use stronger check for PropertyEffects clients. Fixes #5107 #5108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6d558e
Use stronger check for PropertyEffects clients. Fixes #5017
kevinpschaaf Feb 8, 2018
ef579e2
Update eslint parserOptions to es2017 for async/await support.
kevinpschaaf Feb 8, 2018
dea9080
Revert types.
kevinpschaaf Feb 8, 2018
d4a7a45
Revert use of async/await due to lack of build/serve support.
kevinpschaaf Feb 9, 2018
6b3e007
Avoid timing issues with polyfilled Promise
kevinpschaaf Feb 9, 2018
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
7 changes: 6 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 @@ -567,6 +567,7 @@
// e.g.: foo="{{obj.sub}}", path: 'obj.sub.prop', set 'foo.prop'=obj.sub.prop
if (hasPaths && part.source && (path.length > part.source.length) &&
(binding.kind == 'property') && !binding.isCompound &&
node.__isPropertyEffectsClient &&
node.__dataHasAccessor && node.__dataHasAccessor[binding.target]) {
let value = props[path];
path = Polymer.Path.translate(part.source, binding.target, path);
Expand Down Expand Up @@ -603,7 +604,8 @@
} else {
// Property binding
let prop = binding.target;
if (node.__dataHasAccessor && node.__dataHasAccessor[prop]) {
if (node.__isPropertyEffectsClient &&
node.__dataHasAccessor && node.__dataHasAccessor[prop]) {
if (!node[TYPES.READ_ONLY] || !node[TYPES.READ_ONLY][prop]) {
if (node._setPendingProperty(prop, value)) {
inst._enqueueClient(node);
Expand Down Expand Up @@ -1131,6 +1133,9 @@

constructor() {
super();
/** @type {boolean} */
// Used to identify users of this mixin, ala instanceof
this.__isPropertyEffectsClient = true;
/** @type {number} */
// NOTE: used to track re-entrant calls to `_flushProperties`
// path changes dirty check against `__dataTemp` only during one "turn"
Expand Down
28 changes: 28 additions & 0 deletions test/unit/path-effects-elements.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,32 @@
Code distributed by Google as part of the polymer project is also
subject to an additional IP rights grant found at http://polymer.github.io/PATENTS.txt
-->

<link rel="import" href="../../lib/mixins/properties-mixin.html">

<script>
class XPropertiesElement extends Polymer.PropertiesMixin(HTMLElement) {
static get properties() {
return {
obj: Object
};
}
constructor() {
super();
this.resetObservers();
}
// Allow re-set objects to show in _propertiesChanged
_shouldPropertyChange(property, value, old) {
return (typeof value == 'object') ||
super._shouldPropertyChange(property, value, old);
}
resetObservers() {
this._propertiesChanged = sinon.spy();
}
}
customElements.define('x-pe', XPropertiesElement);
</script>

<script>
Polymer({
is: 'x-basic',
Expand Down Expand Up @@ -103,6 +129,7 @@
<x-compose id="compose" obj="{{nested.obj}}"></x-compose>
<x-forward id="forward" obj="{{nested.obj}}"></x-forward>
<div id="boundChild" computed-from-paths="{{computeFromPaths(a, nested.b, nested.obj.c)}}" array-length="{{data.length}}"></div>
<x-pe id="pe" obj="[[nested.obj]]"></x-pe>
</template>
<script>
Polymer({
Expand Down Expand Up @@ -161,6 +188,7 @@
if (this.$) {
this.$.compose.resetObservers();
this.$.forward.resetObservers();
this.$.pe.resetObservers();
}
},
computeFromPaths: function(a, b, c) {
Expand Down
67 changes: 44 additions & 23 deletions test/unit/path-effects.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
document.body.removeChild(el);
});

function verifyObserverOutput(nestingLevel, nested) {
function verifyObserverOutput(nestingLevel, nested, done) {
assert.equal(el.computed, '[' + nested.obj.value + ']');
assert.equal(el.nestedChanged.callCount, nestingLevel == 0 ? 1 : 0);
if (nestingLevel == 0) {
Expand Down Expand Up @@ -92,6 +92,7 @@
assert.equal(el.$.compose.$.basic2.getAttribute('attrvalue'), '42');
assert.equal(el.$.forward.$.compose.$.basic1.getAttribute('attrvalue'), '42');
assert.equal(el.$.forward.$.compose.$.basic2.getAttribute('attrvalue'), '42');
assert.equal(el.$.pe.obj, nested.obj);
switch (nestingLevel) {
case 0:
assert.equal(el.nestedSubpathChanged.firstCall.args[0].path, 'nested');
Expand All @@ -104,6 +105,12 @@
assert.equal(el.$.forward.objSubpathChanged.firstCall.args[0].value, nested.obj);
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].path, 'obj');
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].value, nested.obj);
// x-pe uses async PropertiesChanged
setTimeout(() => {
assert.equal(el.$.pe._propertiesChanged.callCount, 1);
assert.equal(el.$.pe._propertiesChanged.firstCall.args[1].obj, nested.obj);
done();
});
break;
case 1:
assert.equal(el.nestedSubpathChanged.firstCall.args[0].path, 'nested.obj');
Expand All @@ -116,6 +123,12 @@
assert.equal(el.$.forward.objSubpathChanged.firstCall.args[0].value, nested.obj);
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].path, 'obj');
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].value, nested.obj);
// x-pe uses async PropertiesChanged
setTimeout(() => {
assert.equal(el.$.pe._propertiesChanged.callCount, 1);
assert.equal(el.$.pe._propertiesChanged.firstCall.args[1].obj, nested.obj);
done();
});
break;
case 2:
assert.equal(el.nestedSubpathChanged.firstCall.args[0].path, 'nested.obj.value');
Expand All @@ -128,22 +141,30 @@
assert.equal(el.$.forward.objSubpathChanged.firstCall.args[0].value, 42);
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].path, 'obj.value');
assert.equal(el.$.forward.$.compose.objSubpathChanged.firstCall.args[0].value, 42);
// x-pe uses async PropertiesChanged
setTimeout(() => {
// Note the PropertiesMixin element only sees a deep set to 'nested.obj.value'
// because it overrides _shouldPropertyChange to allow object re-sets through
assert.equal(el.$.pe._propertiesChanged.callCount, 1);
assert.equal(el.$.pe._propertiesChanged.firstCall.args[1].obj, nested.obj);
done();
});
break;
}
}

test('downward data flow', function() {
test('downward data flow', function(done) {
// Do the thing
el.nested = {
obj: {
value: 42
}
};
// Verify
verifyObserverOutput(0, el.nested);
verifyObserverOutput(0, el.nested, done);
});

test('notification from basic element property change', function() {
test('notification from basic element property change', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -154,10 +175,10 @@
// Do the thing
el.$.basic.notifyingValue = 42;
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from composed element property change', function() {
test('notification from composed element property change', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -168,10 +189,10 @@
// Do the thing
el.$.compose.$.basic1.notifyingValue = 42;
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from forward\'s composed element property change', function() {
test('notification from forward\'s composed element property change', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -182,10 +203,10 @@
// Do the thing
el.$.forward.$.compose.$.basic1.notifyingValue = 42;
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from set in top element', function() {
test('notification from set in top element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -196,10 +217,10 @@
// Do the thing
el.set('nested.obj.value', 42);
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from set in composed element', function() {
test('notification from set in composed element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -210,10 +231,10 @@
// Do the thing
el.$.compose.set('obj.value', 42);
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from set in forward element', function() {
test('notification from set in forward element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -224,10 +245,10 @@
// Do the thing
el.$.forward.set('obj.value', 42);
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from set in forward\'s composed element', function() {
test('notification from set in forward\'s composed element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -238,10 +259,10 @@
// Do the thing
el.$.forward.$.compose.set('obj.value', 42);
// Verify
verifyObserverOutput(2, el.nested);
verifyObserverOutput(2, el.nested, done);
});

test('notification from object change in compose element', function() {
test('notification from object change in compose element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -254,10 +275,10 @@
value: 42
};
// Verify
verifyObserverOutput(1, el.nested);
verifyObserverOutput(1, el.nested, done);
});

test('notification from object change in forward element', function() {
test('notification from object change in forward element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -270,10 +291,10 @@
value: 42
};
// Verify
verifyObserverOutput(1, el.nested);
verifyObserverOutput(1, el.nested, done);
});

test('notification from object change in forward\'s compose element', function() {
test('notification from object change in forward\'s compose element', function(done) {
// Setup
el.nested = {
obj: {
Expand All @@ -287,7 +308,7 @@
value: 42
};
// Verify
verifyObserverOutput(1, el.nested);
verifyObserverOutput(1, el.nested, done);
});

test('cached paths get invalidated by object sets', function() {
Expand Down