Cleanup, add tests.

* remove old implementation
* add API docs
* rename some API
* add dynamicFn to dep count
* add test for method as dependency

Author: kevinpschaaf

lib/mixins/property-effects.js

@@ -415,45 +415,59 @@ function runReflectEffect(inst, property, props, oldProps, info) {
 function runComputedEffects(inst, changedProps, oldProps, hasPaths) {
   let computeEffects = inst[TYPES.COMPUTE];
   if (computeEffects) {
-    if (orderedComputed == 2) {
+    if (orderedComputed) {
+      // Runs computed effects in efficient order by keeping a topologically-
+      // sorted queue of compute effects to run, and inserting subsequently
+      // invalidated effects as they are run
       dedupeId++;
-      const order = orderedComputedDeps(inst);
+      const order = getComputedOrder(inst);
       const queue = [];
       for (let p in changedProps) {
-        addEffectsFor(p, computeEffects, queue, order, hasPaths);
+        enqueueEffectsFor(p, computeEffects, queue, order, hasPaths);
       }
       let info;
       while ((info = queue.shift())) {
         if (runComputedEffect(inst, '', changedProps, oldProps, info, hasPaths)) {
-          addEffectsFor(info.methodInfo, computeEffects, queue, order, hasPaths);
+          enqueueEffectsFor(info.methodInfo, computeEffects, queue, order, hasPaths);
         }
       }
       Object.assign(/** @type {!Object} */ (oldProps), inst.__dataOld);
       Object.assign(/** @type {!Object} */ (changedProps), inst.__dataPending);
       inst.__dataPending = null;
     } else {
+      // Original Polymer 2.x computed effects order, which continues running
+      // effects until no further computed properties have been invalidated
       let inputProps = changedProps;
       while (runEffects(inst, computeEffects, inputProps, oldProps, hasPaths)) {
         Object.assign(/** @type {!Object} */ (oldProps), inst.__dataOld);
         Object.assign(/** @type {!Object} */ (changedProps), inst.__dataPending);
         inputProps = inst.__dataPending;
         inst.__dataPending = null;
-        if (orderedComputed) {
-          // Ensure all computed properties are de-duped against the same turn
-          dedupeId--;
-        }
       }
     }
   }
 }
 
-const insertEffect = (info, effects, order) => {
+/**
+ * Inserts a computed effect into a queue, given the specified order. Performs
+ * the insert using a binary search.
+ * 
+ * Used by `orderedComputed: true` computed property algorithm.
+ * 
+ * @param {Object} info Property effects metadata
+ * @param {Array<Object>} queue Ordered queue of effects
+ * @param {Map<string,number>} order Map of computed property name->topological
+ *   sort order
+ */
+const insertEffect = (info, queue, order) => {
   let start = 0;
-  let end = effects.length - 1;
+  let end = queue.length - 1;
   let idx = -1;
   while (start <= end) {
     const mid = (start + end) >> 1;
-    const cmp = order.get(effects[mid].methodInfo) - order.get(info.methodInfo);
+    // Note `methodInfo` is where the computed property name is stored in 
+    // the effect metadata
+    const cmp = order.get(queue[mid].methodInfo) - order.get(info.methodInfo);
     if (cmp < 0) {
       start = mid + 1;
     } else if (cmp > 0) {
@@ -466,10 +480,24 @@ const insertEffect = (info, effects, order) => {
   if (idx < 0) {
     idx = end + 1;
   }
-  effects.splice(idx, 0, info);
+  queue.splice(idx, 0, info);
 };
 
-const addEffectsFor = (prop, computeEffects, queue, order, hasPaths) => {
+/**
+ * Inserts all downstream computed effects invalidated by the specified property
+ * into the topologically-sorted queue of effects to be run.
+ * 
+ * Used by `orderedComputed: true` computed property algorithm.
+ * 
+ * @param {string} prop Property name
+ * @param {Object} computeEffects Computed effects for this element
+ * @param {Array<Obbject>} queue Topologically-sorted queue of computed effects
+ *   to be run
+ * @param {Map<string,numer>} order Map of computed property name->topological
+ *   sort order
+ * @param {boolean} hasPaths True with `changedProps` contains one or more paths
+ */
+const enqueueEffectsFor = (prop, computeEffects, queue, order, hasPaths) => {
   const rootProperty = hasPaths ? root(prop) : prop;
   const fxs = computeEffects[rootProperty];
   if (fxs) {
@@ -484,21 +512,43 @@ const addEffectsFor = (prop, computeEffects, queue, order, hasPaths) => {
   }
 };
 
-function orderedComputedDeps(inst) {
+/**
+ * Generates and retrieves a memoized map of computed property name to its
+ * topologically-sorted order. 
+ *
+ * The map is generated by first assigning a "dependency count" to each property
+ * (defined as number properties it depends on, including its method for
+ * "dynamic functions"). Any properties that have no dependencies are added to
+ * the `ready` queue, which are properties whose order can be added to the final
+ * order map. Properties are popped off the `ready` one by one and a.) added as
+ * the next property in the order map, and b.) each property that it is a
+ * dependency for has its dep count decremented (and if that property's dep
+ * count goes to zero, it is added to the `ready` queue), until all properties
+ * have been visited and ordered.
+ *
+ * Used by `orderedComputed: true` computed property algorithm.
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to retrieve the computed
+ *   effect order for.
+ * @return {Map<string,numbber>} Map of computed property name->topological sort
+ *   order 
+ */
+function getComputedOrder(inst) {
   let ordered = inst.constructor.__orderedComputedDeps;
   if (!ordered) {
     ordered = new Map();
     const effects = inst[TYPES.COMPUTE];
-    const {counts, next} = edgeCounts(inst);
+    const {counts, ready} = dependencyCounts(inst);
     let curr;
-    while ((curr = next.pop())) {
+    while ((curr = ready.shift())) {
       ordered.set(curr, ordered.size);
       const computedByCurr = effects[curr];
       if (computedByCurr) {
         computedByCurr.forEach(fx => {
-          const p = fx.info.methodInfo;
-          if (--counts[p] === 0) {
-            next.push(p);
+          // Note `methodInfo` is where the computed property name is stored
+          const computedProp = fx.info.methodInfo;
+          if (--counts[computedProp] === 0) {
+            ready.push(computedProp);
           }
         });
       }
@@ -508,24 +558,37 @@ function orderedComputedDeps(inst) {
   return ordered;
 }
 
-function edgeCounts(inst) {
+/**
+ * Generates a map of property-to-dependency count (`counts`, where "dependency
+ * count" is the number of dependencies a given property has assuming it is a
+ * computed property, otherwise 0).  It also returns a pre-populated list of
+ * `ready` properties that have no dependencies.
+ *
+ * Used by `orderedComputed: true` computed property algorithm.
+ *
+ * @param {!Polymer_PropertyEffects} inst The instance to generate dependency
+ *   counts for.
+ * @return {Object} Object containing `counts` map (property-to-dependency
+ *   count) and pre-populated `ready` array of properties that had zero
+ *   dependencies.
+ */
+function dependencyCounts(inst) {
   const props = inst.constructor._properties;
-  const depsForComputed = inst[COMPUTE_INFO];
+  const infoForComputed = inst[COMPUTE_INFO];
   const counts = {};
-  const next = [];
+  const ready = [];
   for (let p in props) {
-    const deps = depsForComputed[p];  
-    if (deps) { 
-      counts[p] = deps.args.length;
+    const info = infoForComputed[p];  
+    if (info) { 
+      // Be sure to add the method name itself in case of "dynamic functions"
+      counts[p] = info.args.length + (info.dynamicFn ? 1 : 0);
     } else {
-      next.push(p);
+      ready.push(p);
     }
   }
-  return {counts, next};
+  return {counts, ready};
 }
 
-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
@@ -540,120 +603,19 @@ const TRANSITIVE_DEPENDENCY = '~transitive~dependency~';
  * @return {boolean} True when the property being computed changed
  * @private
  */
-function runComputedEffect(inst, property, changedProps, oldProps, info, hasPaths) {
-  if (orderedComputed == 1) {
-    // 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, hasPaths);
-    // 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, hasPaths)) {
-      return;
-    }
-  }
+function runComputedEffect(inst, property, changedProps, oldProps, info) {
   // 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;
+    return false;
   }
   let computedProp = info.methodInfo;
   if (inst.__dataHasAccessor && inst.__dataHasAccessor[computedProp]) {
     return inst._setPendingProperty(computedProp, result, true);
   } else {
     inst[computedProp] = result;
-  }
-}
-
-/**
- * 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
- * @param {boolean} hasPaths True with `changedProps` contains one or more paths
- * @return {void}
- * @private
- */
- function computeDependencies(inst, info, changedProps, oldProps, hasPaths) {
-  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, hasPaths);
-      }
-    });
-  }
-}
-
-/**
- * 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
- * @param {boolean} hasPaths True with `changedProps` contains one or more paths
- * @return {boolean} true when the computed effect should be re-calculated
- * @private
- */
-function computedPropertyIsDirty(inst, info, changedProps, hasPaths) {
-  return info.dynamicFn && propertyIsDirty(inst, {rootProperty: info.methodName}, changedProps, hasPaths) ||
-    info.args.some(arg => propertyIsDirty(inst, arg, changedProps, hasPaths));
-}
-
-/**
- * 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
- * @param {boolean} hasPaths True with `changedProps` contains one or more paths
- * @return {boolean} true when the property is dirty
- * @private
- */
-function propertyIsDirty(inst, trigger, changedProps, hasPaths) {
-  if (hasPaths) {
-    [changedProps, inst.__dataPending].some(props => {
-      for (let p in props) {
-        if (pathMatchesTrigger(p, trigger)) {
-          return true;
-        }
-      }
-    });
     return false;
-  } else {
-    return Boolean(changedProps && trigger.rootProperty in changedProps ||
-      inst.__dataPending && trigger.rootProperty in inst.__dataPending);
   }
 }
 

test/runner.html

@@ -33,8 +33,10 @@
       'unit/property-effects.html',
       'unit/property-effects.html?legacyUndefined=true',
       'unit/property-effects.html?legacyUndefined=true&legacyNoBatch=true',
+      'unit/property-effects.html?orderedComputed=true',
       'unit/property-effects-template.html',
       'unit/path-effects.html',
+      'unit/path-effects.html?orderedComputed=true',
       'unit/shady.html',
       'unit/shady-events.html',
       'unit/shady-content.html',

test/unit/path-effects.html

@@ -21,6 +21,9 @@
 <script type="module">
 import './path-effects-elements.js';
 import { Polymer } from '../../lib/legacy/polymer-fn.js';
+import { setOrderedComputed } from '../../lib/utils/settings.js';
+
+setOrderedComputed(Boolean(window.location.search.match('orderedComputed')));
 
 // Safari 9 has a horrible bug related to array enumeration after defining
 // a non-enumerable property on it (we do for `.splices`); for now we work

test/unit/property-effects-elements.js

@@ -1014,16 +1014,22 @@ 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},
+      a: {type: Number, value: 1000},
+      b: {type: Number, value: 100},
+      c: {type: Number, value: 10},
+      d: {type: Number, value: 1},
       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)'},
+
+      x: {type: Number, value: 2},
+      y: {type: Number, value: 20},
+      z: {type: Number, value: 200},
+      xy: {computed: 'computeXY(x, y)', observer: 'xyChanged'},
+      computeXY: {computed: 'computeComputeXY(z)'}
     };
   }
   constructor() {
@@ -1034,6 +1040,9 @@ customElements.define('x-computed-ordering', class extends PolymerElement {
     this.abbcdChanged = sinon.spy();
     this.bcdChanged = sinon.spy();
     this.bcChanged = sinon.spy();
+
+    this.computeXYSpy = sinon.spy();
+    this.xyChanged = sinon.spy();
   }
   computeABBCD(a, b, bcd) {
     return a + b + bcd;
@@ -1050,6 +1059,12 @@ customElements.define('x-computed-ordering', class extends PolymerElement {
   computeCircB(circIn, circA) {
     return circIn + (circA || 0);
   }
+  computeComputeXY(z) {
+    return function computeYZ(x, y) {
+      this.computeXYSpy(x, y);
+      return x + y + z;
+    };
+  }
 });
 
 class SVGElement extends PolymerElement {