From e2d170206f84740a05fb8f99e4a507122d7b87bb Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Tue, 18 Apr 2017 12:52:39 -0700 Subject: [PATCH 1/3] Process paths regardless of accessor, & loop on computeLinkedPaths. Fixes #4542 --- lib/mixins/property-effects.html | 60 +++++++++++++------------------- test/unit/path-effects.html | 8 +++-- 2 files changed, 31 insertions(+), 37 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 7241725d1f..560d9cf312 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -371,12 +371,13 @@ */ function runComputedEffects(inst, changedProps, oldProps, hasPaths) { let computeEffects = inst.__computeEffects; - if (computeEffects) { - let inputProps = changedProps; - while (runEffects(inst, computeEffects, inputProps, oldProps, hasPaths)) { + let inputProps = changedProps; + while (inputProps) { + computeEffects && runEffects(inst, computeEffects, inputProps, oldProps, hasPaths); + computeLinkedPaths(inst, inputProps, hasPaths); + if ((inputProps = inst.__dataPending)) { Object.assign(oldProps, inst.__dataOld); Object.assign(changedProps, inst.__dataPending); - inputProps = inst.__dataPending; inst.__dataPending = null; } } @@ -416,21 +417,16 @@ function computeLinkedPaths(inst, changedProps, hasPaths) { let links; if (hasPaths && (links = inst.__dataLinkedPaths)) { - const cache = inst.__dataTemp; let link; for (let a in links) { let b = links[a]; for (let path in changedProps) { if (Polymer.Path.isDescendant(a, path)) { link = Polymer.Path.translate(a, b, path); - cache[link] = changedProps[link] = changedProps[path]; - let notifyProps = inst.__dataToNotify || (inst.__dataToNotify = {}); - notifyProps[link] = true; + inst._setPendingPropertyOrPath(link, changedProps[path], true, true); } else if (Polymer.Path.isDescendant(b, path)) { link = Polymer.Path.translate(b, a, path); - cache[link] = changedProps[link] = changedProps[path]; - let notifyProps = inst.__dataToNotify || (inst.__dataToNotify = {}); - notifyProps[link] = true; + inst._setPendingPropertyOrPath(link, changedProps[path], true, true); } } } @@ -1292,32 +1288,28 @@ */ _setPendingPropertyOrPath(path, value, shouldNotify, isPathNotification) { let rootProperty = Polymer.Path.root(Array.isArray(path) ? path[0] : path); - let hasAccessor = this.__dataHasAccessor && this.__dataHasAccessor[rootProperty]; - let isPath = (rootProperty !== path); - if (hasAccessor) { - if (isPath) { - if (!isPathNotification) { - // Dirty check changes being set to a path against the actual object, - // since this is the entry point for paths into the system; from here - // the only dirty checks are against the `__dataTemp` cache to prevent - // duplicate work in the same turn only. Note, if this was a notification - // of a change already set to a path (isPathNotification: true), - // we always let the change through and skip the `set` since it was - // already dirty checked at the point of entry and the underlying - // object has already been updated - let old = Polymer.Path.get(this, path); - path = /** @type {string} */ (Polymer.Path.set(this, path, value)); - // Use property-accessor's simpler dirty check - if (!path || !super._shouldPropertyChange(path, value, old)) { - return false; - } + if (rootProperty !== path) { + if (!isPathNotification) { + // Dirty check changes being set to a path against the actual object, + // since this is the entry point for paths into the system; from here + // the only dirty checks are against the `__dataTemp` cache to prevent + // duplicate work in the same turn only. Note, if this was a notification + // of a change already set to a path (isPathNotification: true), + // we always let the change through and skip the `set` since it was + // already dirty checked at the point of entry and the underlying + // object has already been updated + let old = Polymer.Path.get(this, path); + path = /** @type {string} */ (Polymer.Path.set(this, path, value)); + // Use property-accessor's simpler dirty check + if (!path || !super._shouldPropertyChange(path, value, old)) { + return false; } - this.__dataHasPaths = true; } + this.__dataHasPaths = true; return this._setPendingProperty(path, value, shouldNotify); } else { - if (isPath) { - Polymer.Path.set(this, path, value); + if (this.__dataHasAccessor && this.__dataHasAccessor[rootProperty]) { + return this._setPendingProperty(path, value, shouldNotify); } else { this[path] = value; } @@ -1548,8 +1540,6 @@ this.__dataHasPaths = false; // Compute properties runComputedEffects(this, changedProps, oldProps, hasPaths); - // Compute linked paths - computeLinkedPaths(this, changedProps, hasPaths); // Clear notify properties prior to possible reentry (propagate, observe), // but after computing effects have a chance to add to them let notifyProps = this.__dataToNotify; diff --git a/test/unit/path-effects.html b/test/unit/path-effects.html index b2db181140..c208b2d8b9 100644 --- a/test/unit/path-effects.html +++ b/test/unit/path-effects.html @@ -898,11 +898,15 @@ assert.equal(el.xChanged.callCount, 1); assert.equal(el.yChanged.callCount, 1); assert.equal(el.aChanged.callCount, 1); - el.unlinkPaths('y'); el.set('a.foo', 2); assert.equal(el.xChanged.callCount, 2); - assert.equal(el.yChanged.callCount, 1); + assert.equal(el.yChanged.callCount, 2); assert.equal(el.aChanged.callCount, 2); + el.unlinkPaths('y'); + el.set('a.foo', 3); + assert.equal(el.xChanged.callCount, 3); + assert.equal(el.yChanged.callCount, 2); + assert.equal(el.aChanged.callCount, 3); }); test('link two arrays', function() { From d722cb9c41b8975b2e5bd8c2cbf520d75822390d Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 19 Apr 2017 10:17:43 -0700 Subject: [PATCH 2/3] Move computeLinkedPaths out of hot path and into sync setter. --- lib/mixins/property-effects.html | 38 ++++++++++++++++---------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index 560d9cf312..cf8b22088d 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -371,13 +371,12 @@ */ function runComputedEffects(inst, changedProps, oldProps, hasPaths) { let computeEffects = inst.__computeEffects; - let inputProps = changedProps; - while (inputProps) { - computeEffects && runEffects(inst, computeEffects, inputProps, oldProps, hasPaths); - computeLinkedPaths(inst, inputProps, hasPaths); - if ((inputProps = inst.__dataPending)) { + if (computeEffects) { + let inputProps = changedProps; + while (runEffects(inst, computeEffects, inputProps, oldProps, hasPaths)) { Object.assign(oldProps, inst.__dataOld); Object.assign(changedProps, inst.__dataPending); + inputProps = inst.__dataPending; inst.__dataPending = null; } } @@ -410,24 +409,22 @@ * API. * * @param {Element} inst The instance whose props are changing - * @param {Object} changedProps Bag of changed properties - * @param {boolean} hasPaths True with `props` contains one or more paths + * @param {string} path Path that has changed + * @param {*} value Value of changed path * @private */ - function computeLinkedPaths(inst, changedProps, hasPaths) { - let links; - if (hasPaths && (links = inst.__dataLinkedPaths)) { + function computeLinkedPaths(inst, path, value) { + let links = inst.__dataLinkedPaths; + if (links) { let link; for (let a in links) { let b = links[a]; - for (let path in changedProps) { - if (Polymer.Path.isDescendant(a, path)) { - link = Polymer.Path.translate(a, b, path); - inst._setPendingPropertyOrPath(link, changedProps[path], true, true); - } else if (Polymer.Path.isDescendant(b, path)) { - link = Polymer.Path.translate(b, a, path); - inst._setPendingPropertyOrPath(link, changedProps[path], true, true); - } + if (Polymer.Path.isDescendant(a, path)) { + link = Polymer.Path.translate(a, b, path); + inst._setPendingPropertyOrPath(link, value, true, true); + } else if (Polymer.Path.isDescendant(b, path)) { + link = Polymer.Path.translate(b, a, path); + inst._setPendingPropertyOrPath(link, value, true, true); } } } @@ -1306,7 +1303,10 @@ } } this.__dataHasPaths = true; - return this._setPendingProperty(path, value, shouldNotify); + if (this._setPendingProperty(path, value, shouldNotify)) { + computeLinkedPaths(this, path, value); + return true; + } } else { if (this.__dataHasAccessor && this.__dataHasAccessor[rootProperty]) { return this._setPendingProperty(path, value, shouldNotify); From 73df8c5b7397db5adca66bd70b1edd113a47ae44 Mon Sep 17 00:00:00 2001 From: Kevin Schaaf Date: Wed, 19 Apr 2017 11:57:34 -0700 Subject: [PATCH 3/3] Add more tests. --- lib/mixins/property-effects.html | 22 +++++----- test/unit/path-effects-elements.html | 6 +++ test/unit/path-effects.html | 63 ++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 11 deletions(-) diff --git a/lib/mixins/property-effects.html b/lib/mixins/property-effects.html index cf8b22088d..dcc1dec0cc 100644 --- a/lib/mixins/property-effects.html +++ b/lib/mixins/property-effects.html @@ -1284,17 +1284,17 @@ * @protected */ _setPendingPropertyOrPath(path, value, shouldNotify, isPathNotification) { - let rootProperty = Polymer.Path.root(Array.isArray(path) ? path[0] : path); - if (rootProperty !== path) { + if (isPathNotification || + Polymer.Path.root(Array.isArray(path) ? path[0] : path) !== path) { + // Dirty check changes being set to a path against the actual object, + // since this is the entry point for paths into the system; from here + // the only dirty checks are against the `__dataTemp` cache to prevent + // duplicate work in the same turn only. Note, if this was a notification + // of a change already set to a path (isPathNotification: true), + // we always let the change through and skip the `set` since it was + // already dirty checked at the point of entry and the underlying + // object has already been updated if (!isPathNotification) { - // Dirty check changes being set to a path against the actual object, - // since this is the entry point for paths into the system; from here - // the only dirty checks are against the `__dataTemp` cache to prevent - // duplicate work in the same turn only. Note, if this was a notification - // of a change already set to a path (isPathNotification: true), - // we always let the change through and skip the `set` since it was - // already dirty checked at the point of entry and the underlying - // object has already been updated let old = Polymer.Path.get(this, path); path = /** @type {string} */ (Polymer.Path.set(this, path, value)); // Use property-accessor's simpler dirty check @@ -1308,7 +1308,7 @@ return true; } } else { - if (this.__dataHasAccessor && this.__dataHasAccessor[rootProperty]) { + if (this.__dataHasAccessor && this.__dataHasAccessor[path]) { return this._setPendingProperty(path, value, shouldNotify); } else { this[path] = value; diff --git a/test/unit/path-effects-elements.html b/test/unit/path-effects-elements.html index 0def2656a3..b409c254b6 100644 --- a/test/unit/path-effects-elements.html +++ b/test/unit/path-effects-elements.html @@ -114,6 +114,9 @@ computedFromPaths: { computed: 'computeFromPaths(a, nested.b, nested.obj.c)' }, + computedFromLinkedPaths: { + computed: 'computeFromLinkedPaths(a, linked1.prop, linked2.prop)' + }, computed: { computed: 'compute(nested.obj.value)' } @@ -163,6 +166,9 @@ computeFromPaths: function(a, b, c) { return a + b + c; }, + computeFromLinkedPaths: sinon.spy(function(a, b, c) { + return a + b + c; + }), compute: function(val) { return '[' + val + ']'; } diff --git a/test/unit/path-effects.html b/test/unit/path-effects.html index c208b2d8b9..f432e531dd 100644 --- a/test/unit/path-effects.html +++ b/test/unit/path-effects.html @@ -909,6 +909,69 @@ assert.equal(el.aChanged.callCount, 3); }); + test('multiple linked dependencies to computed property', function() { + let linkedObj = {prop: 'Linked'}; + el.computeFromLinkedPaths.reset(); + el.setProperties({ + linked1: linkedObj, + linked2: linkedObj, + a: 'A' + }); + assert.equal(el.computeFromLinkedPaths.callCount, 1); + assert.equal(el.computedFromLinkedPaths, 'ALinkedLinked'); + + el.linkPaths('linked1', 'linked2'); + el.set('linked1.prop', 'Linked+'); + assert.equal(el.computeFromLinkedPaths.callCount, 2); + assert.equal(el.computedFromLinkedPaths, 'ALinked+Linked+'); + el.linked3 = el.linked2; + el.linkPaths('linked2', 'linked3'); + el.set('linked3.prop', 'Linked++'); + assert.equal(el.computeFromLinkedPaths.callCount, 3); + assert.equal(el.computedFromLinkedPaths, 'ALinked++Linked++'); + el.set('linked2.prop', 'Linked+++'); + assert.equal(el.computeFromLinkedPaths.callCount, 4); + assert.equal(el.computedFromLinkedPaths, 'ALinked+++Linked+++'); + el.set('linked1.prop', 'Linked++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 5); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++Linked++++'); + + el.linked4 = el.linked1; + el.linkPaths('linked4', 'linked1'); + el.set('linked4.prop', 'Linked+++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 6); + assert.equal(el.computedFromLinkedPaths, 'ALinked+++++Linked+++++'); + el.set('linked3.prop', 'Linked++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 7); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++++Linked++++++'); + el.set('linked2.prop', 'Linked+++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 8); + assert.equal(el.computedFromLinkedPaths, 'ALinked+++++++Linked+++++++'); + el.set('linked1.prop', 'Linked++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 9); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++++++Linked++++++++'); + + el.linked5 = el.linked3; + el.linkPaths('linked5', 'linked3'); + el.set('linked4.prop', 'Linked+++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 10); + assert.equal(el.computedFromLinkedPaths, 'ALinked+++++++++Linked+++++++++'); + el.set('linked3.prop', 'Linked++++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 11); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++++++++Linked++++++++++'); + el.set('linked2.prop', 'Linked+++++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 12); + assert.equal(el.computedFromLinkedPaths, 'ALinked+++++++++++Linked+++++++++++'); + el.set('linked1.prop', 'Linked++++++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 13); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++++++++++Linked++++++++++++'); + + el.unlinkPaths('linked4'); + el.set('linked4.prop', 'Linked+++++++++++++'); + assert.equal(el.computeFromLinkedPaths.callCount, 13); + assert.equal(el.computedFromLinkedPaths, 'ALinked++++++++++++Linked++++++++++++'); + }); + test('link two arrays', function() { el.x = el.y = []; el.linkPaths('y', 'x');