Skip to content

Commit

Permalink
Evaluate computed property dependencies first. Fixes #5143
Browse files Browse the repository at this point in the history
  • Loading branch information
@kevinpschaaf
kevinpschaaf committed Mar 1, 2019
1 parent 69463da commit 832fcde
Show file tree
Hide file tree
Showing 4 changed files with 271 additions and 15 deletions.
135 changes: 121 additions & 14 deletions lib/mixins/property-effects.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { camelToDashCase, dashToCamelCase } from '../utils/case-map.js';
import { PropertyAccessors } from './property-accessors.js';
/* for annotated effects */
import { TemplateStamp } from './template-stamp.js';
import { sanitizeDOMValue, legacyUndefined, legacyNoBatch, legacyNotifyOrder } from '../utils/settings.js';
import { sanitizeDOMValue, legacyUndefined, legacyNoBatch, legacyNotifyOrder, orderedComputed } from '../utils/settings.js';

// Monotonically increasing unique ID used for de-duping effects triggered
// from multiple properties in the same turn
Expand All @@ -40,6 +40,8 @@ const TYPES = {
READ_ONLY: '__readOnly'
};

const COMPUTE_INFO = '__computeInfo';

/** @const {!RegExp} */
const capitalAttributeRegex = /[A-Z]/;

Expand Down Expand Up @@ -80,20 +82,23 @@ let DataEffect; //eslint-disable-line no-unused-vars
*
* @param {Object} model Prototype or instance
* @param {string} type Property effect type
* @param {boolean=} cloneArrays Clone any arrays mapped in the map
* @return {Object} The own-property map of effects for the given type
* @private
*/
function ensureOwnEffectMap(model, type) {
function ensureOwnEffectMap(model, type, cloneArrays) {
let effects = model[type];
if (!effects) {
effects = model[type] = {};
} else if (!model.hasOwnProperty(type)) {
effects = model[type] = Object.create(model[type]);
for (let p in effects) {
let protoFx = effects[p];
let instFx = effects[p] = Array(protoFx.length);
for (let i=0; i<protoFx.length; i++) {
instFx[i] = protoFx[i];
if (cloneArrays) {
for (let p in effects) {
let protoFx = effects[p];
let instFx = effects[p] = Array(protoFx.length);
for (let i=0; i<protoFx.length; i++) {
instFx[i] = protoFx[i];
}
}
}
}
Expand Down Expand Up @@ -415,25 +420,43 @@ function runComputedEffects(inst, changedProps, oldProps, hasPaths) {
Object.assign(/** @type {!Object} */ (changedProps), inst.__dataPending);
inputProps = inst.__dataPending;
inst.__dataPending = null;
// Ensure all computed properties are de-duped against the same turn
dedupeId--;
}
}
}

const TRANSITIVE_DEPENDENCY = '~transitive~dependency~';

/**
* Implements the "computed property" effect by running the method with the
* values of the arguments specified in the `info` object and setting the
* return value to the computed property specified.
*
* @param {!Polymer_PropertyEffects} inst The instance the effect will be run on
* @param {string} property Name of property
* @param {?Object} props Bag of current property changes
* @param {?Object} changedProps Bag of current property changes
* @param {?Object} oldProps Bag of previous values for changed properties
* @param {?} info Effect metadata
* @return {void}
* @private
*/
function runComputedEffect(inst, property, props, oldProps, info) {
let result = runMethodEffect(inst, property, props, oldProps, info);
function runComputedEffect(inst, property, changedProps, oldProps, info) {
if (orderedComputed) {
// Compute any computed dependencies first; this recurses through the
// dependency graph to ensure computed properties are never computed with
// dependencies that may become invalidated later in this turn
computeDependencies(inst, info, changedProps, oldProps);
// If the dependency was not transitive, it's definitely dirty and needs to
// be computed; if it is transitive, check its arguments against the current
// changed props and only re-compute if it is dirty
if (property === TRANSITIVE_DEPENDENCY &&
!computedPropertyIsDirty(inst, info, changedProps)) {
return;
}
}
// Dirty check dependencies and run if any invalid
let result = runMethodEffect(inst, property, changedProps, oldProps, info);
// Abort if method returns a no-op result
if (result === NOOP) {
return;
Expand All @@ -446,6 +469,86 @@ function runComputedEffect(inst, property, props, oldProps, info) {
}
}

/**
* Returns any dependencies of a computed property that are themselves
* also computed.
*
* @param {!Polymer_PropertyEffects} inst The instance to test
* @param {?} info Computed effect metadata
* @return {Array<?>} Array of computed effect metadata for depenencies
* @private
*/
function computedDependenciesFor(inst, info) {
let deps = info.computedDependencies;
if (!deps) {
deps = info.computedDependencies = [];
info.args.forEach(arg => {
const dep = arg.rootProperty || arg.name;
const depInfo = inst[COMPUTE_INFO][dep];
if (depInfo) {
deps.push(depInfo);
}
});
}
return deps;
}

/**
* Runs computed effects for dependencies of the computed property effect
* described by `info`.
*
* @param {!Polymer_PropertyEffects} inst The instance to test
* @param {Object} info Effect metadata
* @param {Object} changedProps Bag of current property changes
* @param {Object} oldProps Bag of previous values for changed properties
* @return {void}
* @private
*/
function computeDependencies(inst, info, changedProps, oldProps) {
let deps = computedDependenciesFor(inst, info);
if (deps.length) {
deps.forEach(depInfo => {
if (depInfo.lastRun !== info.lastRun) {
depInfo.lastRun = info.lastRun;
runComputedEffect(inst, TRANSITIVE_DEPENDENCY, changedProps, oldProps, depInfo);
}
});
}
}

/**
* Returns whether the computed effect has any changed dependencies.
*
* @param {!Polymer_PropertyEffects} inst The instance to test
* @param {?} info Effect metadata
* @param {Object} changedProps Bag of current property changes
* @return {boolean} true when the computed effect should be re-calculated
* @private
*/
function computedPropertyIsDirty(inst, info, changedProps) {
return info.dynamicFn && propertyIsDirty(inst, {name: info.methodName}, changedProps) ||
info.args.some(arg => propertyIsDirty(inst, arg, changedProps));
}

/**
* Returns whether any changed props match the effect trigger
*
* @param {!Polymer_PropertyEffects} inst The instance to test
* @param {DataTrigger} trigger Descriptor
* @param {Object} changedProps Bag of current property changes
* @return {boolean} true when the property is dirty
* @private
*/
function propertyIsDirty(inst, trigger, changedProps) {
return [changedProps, inst.__dataPending].some(props => {
for (let p in props) {
if (pathMatchesTrigger(p, trigger)) {
return true;
}
}
});
}

/**
* Computes path changes based on path links set up using the `linkPaths`
* API.
Expand Down Expand Up @@ -778,7 +881,7 @@ function addNotifyListener(node, inst, binding) {
* @param {boolean|Object=} dynamicFn Boolean or object map indicating whether
* method names should be included as a dependency to the effect. Note,
* defaults to true if the signature is static (sig.static is true).
* @return {void}
* @return {Object} Effect metadata for this method effect
* @private
*/
function createMethodEffect(model, sig, type, effectFn, methodInfo, dynamicFn) {
Expand All @@ -802,6 +905,7 @@ function createMethodEffect(model, sig, type, effectFn, methodInfo, dynamicFn) {
fn: effectFn, info: info
});
}
return info;
}

/**
Expand Down Expand Up @@ -1152,6 +1256,8 @@ export const PropertyEffects = dedupingMixin(superClass => {
/** @type {Object} */
this.__computeEffects;
/** @type {Object} */
this.__computeInfo;
/** @type {Object} */
this.__reflectEffects;
/** @type {Object} */
this.__notifyEffects;
Expand Down Expand Up @@ -1239,7 +1345,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
_addPropertyEffect(property, type, effect) {
this._createPropertyAccessor(property, type == TYPES.READ_ONLY);
// effects are accumulated into arrays per property based on type
let effects = ensureOwnEffectMap(this, type)[property];
let effects = ensureOwnEffectMap(this, type, true)[property];
if (!effects) {
effects = this[type][property] = [];
}
Expand All @@ -1256,7 +1362,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
* @return {void}
*/
_removePropertyEffect(property, type, effect) {
let effects = ensureOwnEffectMap(this, type)[property];
let effects = ensureOwnEffectMap(this, type, true)[property];
let idx = effects.indexOf(effect);
if (idx >= 0) {
effects.splice(idx, 1);
Expand Down Expand Up @@ -2249,7 +2355,8 @@ export const PropertyEffects = dedupingMixin(superClass => {
if (!sig) {
throw new Error("Malformed computed expression '" + expression + "'");
}
createMethodEffect(this, sig, TYPES.COMPUTE, runComputedEffect, property, dynamicFn);
const info = createMethodEffect(this, sig, TYPES.COMPUTE, runComputedEffect, property, dynamicFn);
ensureOwnEffectMap(this, COMPUTE_INFO)[property] = info;
}

/**
Expand Down
17 changes: 17 additions & 0 deletions lib/utils/settings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,3 +228,20 @@ export let legacyNotifyOrder = false;
export const setLegacyNotifyOrder = function(useLegacyNotifyOrder) {
legacyNotifyOrder = useLegacyNotifyOrder;
};

/**
* Setting to ensure computed properties are computed in order to ensure
* re-computation never occurs in a given turn.
*/
export let orderedComputed = false;

/**
* Sets `orderedComputed` globally for all elements to enable ordered computed
* property computation.
*
* @param {boolean} useOrderedComputed enable or disable ordered computed effects
* @return {void}
*/
export const setOrderedComputed = function(useOrderedComputed) {
orderedComputed = useOrderedComputed;
};
41 changes: 41 additions & 0 deletions test/unit/property-effects-elements.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,47 @@ class SubObserverElement extends SuperObserverElement {
}
customElements.define(SubObserverElement.is, SubObserverElement);

customElements.define('x-computed-ordering', class extends PolymerElement {
static get properties() {
return {
a: {type: Number},
b: {type: Number},
c: {type: Number},
d: {type: Number},
abbcd: {computed: 'computeABBCD(a, b, bcd)', observer: 'abbcdChanged'},
bcd: {computed: 'computeBCD(bc, d)', observer: 'bcdChanged'},
bc: {computed: 'computeBC(b, c)', observer: 'bcChanged'},
circIn: {type: Number},
circA: {computed: 'computeCircA(circIn, circB)'},
circB: {computed: 'computeCircA(circIn, circA)'},
};
}
constructor() {
super();
sinon.spy(this, 'computeABBCD');
sinon.spy(this, 'computeBCD');
sinon.spy(this, 'computeBC');
this.abbcdChanged = sinon.spy();
this.bcdChanged = sinon.spy();
this.bcChanged = sinon.spy();
}
computeABBCD(a, b, bcd) {
return a + b + bcd;
}
computeBCD(bc, d) {
return bc + d;
}
computeBC(b, c) {
return b + c;
}
computeCircA(circIn, circB) {
return circIn + (circB || 0);
}
computeCircB(circIn, circA) {
return circIn + (circA || 0);
}
});

class SVGElement extends PolymerElement {
static get template() {
return html`
Expand Down
Loading

0 comments on commit 832fcde

Please sign in to comment.