From ae1b4173b0a303c4d16e82c88c1c8757cb594217 Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Fri, 24 Aug 2018 11:45:33 -0700
Subject: [PATCH] Updates based on code review. Add computed tests.

---
 lib/legacy/legacy-data-mixin.js |  73 ++++++++++----
 test/unit/legacy-data.html      | 164 +++++++++++++++++++++++++++++++-
 2 files changed, 212 insertions(+), 25 deletions(-)

diff --git a/lib/legacy/legacy-data-mixin.js b/lib/legacy/legacy-data-mixin.js
index cc9aa2b55b..018ccd31f9 100644
--- a/lib/legacy/legacy-data-mixin.js
+++ b/lib/legacy/legacy-data-mixin.js
@@ -13,15 +13,43 @@ import { Polymer } from '../../polymer-legacy.js';
 import { dedupingMixin } from '../utils/mixin.js';
 
 const UndefinedArgumentError = class extends Error {
-  constructor(message) {
+  constructor(message, arg) {
     super(message);
+    this.arg = arg;
     this.name = this.constructor.name;
     // Affordances for ensuring instanceof works after babel ES5 compilation
+    // TODO(kschaaf): Remove after polymer CLI updates to newer Babel that
+    // sets the constructor/prototype correctly for subclassed builtins
     this.constructor = UndefinedArgumentError;
     this.__proto__ = UndefinedArgumentError.prototype;
   }
 };
 
+/**
+ * Wraps effect functions to catch `UndefinedArgumentError`s and warn.
+ * 
+ * @param {Object=} effect Effect metadata object
+ * @param {Object=} fnName Name of user function, if known
+ * @return {Object=} Effect metadata object
+ */
+function wrapEffect(effect, fnName) {
+  if (effect && effect.fn) {
+    const fn = effect.fn;
+    effect.fn = function() {
+      try {
+        fn.apply(this, arguments);
+      } catch (e) {
+        if (e instanceof UndefinedArgumentError) {
+          console.warn(`Argument '${e.arg}'${fnName ?` for method '${fnName}'` : ''} was undefined. Ensure it has an undefined check.`);
+        } else {
+          throw e;
+        }
+      }
+    };
+  }
+  return effect;
+}
+
 /**
  * Mixin to selectively add back Polymer 1.x's `undefined` rules
  * governing when observers & computing functions run based
@@ -50,7 +78,7 @@ export const LegacyDataMixin = dedupingMixin(superClass => {
    * @private   */
   class LegacyDataMixin extends superClass {
     /**
-     * Overrides `Polyer.PropertyEffects` to add `undefined` argument
+     * Overrides `Polymer.PropertyEffects` to add `undefined` argument
      * checking to match Polymer 1.x style rules
      * 
      * @param {!Array<!MethodArg>} args Array of argument metadata
@@ -61,12 +89,16 @@ export const LegacyDataMixin = dedupingMixin(superClass => {
      */
     _marshalArgs(args, path, props) {
       const vals = super._marshalArgs(args, path, props);
-      if (this._legacyUndefinedCheck && args.length > 1) {
-        for (let i=0; i<args.length; i++) {
+      // Per legacy data rules, single-property observers (whether in `properties`
+      // and in `observers`) are called regardless of whether their argument is
+      // undefined or not. Multi-property observers must have all arguments defined
+      if (this._legacyUndefinedCheck && vals.length > 1) {
+        for (let i=0; i<vals.length; i++) {
           if (vals[i] === undefined) {
             // Break out of effect's control flow; will be caught in
             // wrapped property effect function below
-            throw new UndefinedArgumentError(`argument '${args[i].name}' is undefined; ensure it has an undefined check`);
+            const name = args[i].name;
+            throw new UndefinedArgumentError(`Argument '${name}' is undefined. Ensure it has an undefined check.`, name);
           }
         }
       }
@@ -84,21 +116,22 @@ export const LegacyDataMixin = dedupingMixin(superClass => {
      * @protected
      */
     _addPropertyEffect(property, type, effect) {
-      if (effect && effect.fn) {
-        const fn = effect.fn;
-        effect.fn = function() {
-          try {
-            fn.apply(this, arguments);
-          } catch (e) {
-            if (e instanceof UndefinedArgumentError) {
-              console.warn(e.message);
-            } else {
-              throw e;
-            }
-          }
-        };
-      }
-      return super._addPropertyEffect(property, type, effect);
+      return super._addPropertyEffect(property, type,
+        wrapEffect(effect, effect && effect.info && effect.info.methodName));
+    }
+
+    /**
+     * Overrides `Polyer.PropertyEffects` to wrap effect functions to
+     * catch `UndefinedArgumentError`s and warn.
+     *
+     * @param {Object} templateInfo Template metadata to add effect to
+     * @param {string} prop Property that should trigger the effect
+     * @param {Object=} effect Effect metadata object
+     * @return {void}
+     * @protected
+     */
+    static _addTemplatePropertyEffect(templateInfo, prop, effect) {
+      return super._addTemplatePropertyEffect(templateInfo, prop, wrapEffect(effect));
     }
 
   }
diff --git a/test/unit/legacy-data.html b/test/unit/legacy-data.html
index be01f95109..3747c9f561 100644
--- a/test/unit/legacy-data.html
+++ b/test/unit/legacy-data.html
@@ -20,16 +20,34 @@
 <body>
 
   <dom-module id="x-data">
+    <template>
+      <div id="child"
+        computed-single="[[computeSingle(inlineSingleDep)]]"
+        computed-multi="[[computeMulti(inlineMultiDep1, inlineMultiDep2)]]">
+      </div>
+    </template>
     <script type="module">
       import {Polymer} from '../../polymer-legacy.js';
       Polymer({
         is: 'x-data',
+        _legacyUndefinedCheck: true,
         properties: {
           singleProp: String,
           multiProp1: String,
-          multiProp2: String
+          multiProp2: String,
+          computedSingleDep: String,
+          computedMultiDep1: String,
+          computedMultiDep2: String,
+          inlineSingleDep: String,
+          inlineMultiDep1: String,
+          inlineMultiDep2: String,
+          computedSingle: {
+            computed: 'computeSingle(computedSingleDep)'
+          },
+          computedMulti: {
+            computed: 'computeMulti(computedMultiDep1, computedMultiDep2)'
+          }
         },
-        _legacyUndefinedCheck: true,
         observers: [
           'staticObserver("staticObserver")',
           'singlePropObserver(singleProp)',
@@ -40,6 +58,8 @@
           this.singlePropObserver = sinon.spy();
           this.multiPropObserver = sinon.spy();
           this.staticObserver = sinon.spy();
+          this.computeSingle = sinon.spy((inlineSingleDep) => `[${inlineSingleDep}]`);
+          this.computeMulti = sinon.spy((inlineMultiDep1, inlineMultiDep2) => `[${inlineMultiDep1},${inlineMultiDep2}]`);
         },
         throws() {
           throw new Error('real error');
@@ -72,6 +92,42 @@
     </template>
   </test-fixture>
 
+  <test-fixture id="declarative-single-computed">
+    <template>
+      <x-data computed-single-dep="a"></x-data>
+    </template>
+  </test-fixture>
+
+  <test-fixture id="declarative-multi-one-computed">
+    <template>
+      <x-data computed-multi-dep1="b"></x-data>
+    </template>
+  </test-fixture>
+
+  <test-fixture id="declarative-multi-all-computed">
+    <template>
+      <x-data computed-multi-dep1="b" computed-multi-dep2="c"></x-data>
+    </template>
+  </test-fixture>
+
+  <test-fixture id="declarative-single-computed-inline">
+    <template>
+      <x-data inline-single-dep="a"></x-data>
+    </template>
+  </test-fixture>
+
+  <test-fixture id="declarative-multi-one-computed-inline">
+    <template>
+      <x-data inline-multi-dep1="b"></x-data>
+    </template>
+  </test-fixture>
+
+  <test-fixture id="declarative-multi-all-computed-inline">
+    <template>
+      <x-data inline-multi-dep1="b" inline-multi-dep2="c"></x-data>
+    </template>
+  </test-fixture>
+
   <script>
   (function() {
 
@@ -83,6 +139,10 @@
         callCounts.singlePropObserver || 0, 'singlePropObserver call count wrong');
       assert.equal(el.multiPropObserver.callCount,
         callCounts.multiPropObserver || 0, 'multiPropObserver call count wrong');
+      assert.equal(el.computeSingle.callCount,
+        callCounts.computeSingle || 0, 'computeSingle call count wrong');
+      assert.equal(el.computeMulti.callCount,
+        callCounts.computeMulti || 0, 'computeMulti call count wrong');
       assert.equal(console.warn.callCount, callCounts.warn || 0,
         'console.warn call count wrong');
     }
@@ -103,9 +163,15 @@
         el.parentNode.removeChild(el);
       });
 
-      const singleProp = 'a';
-      const multiProp1 = 'b';
-      const multiProp2 = 'c';
+      const singleProp = 'singleProp';
+      const multiProp1 = 'multiProp1';
+      const multiProp2 = 'multiProp2';
+      const computedSingleDep = 'computedSingleDep';
+      const computedMultiDep1 = 'computedMultiDep1';
+      const computedMultiDep2 = 'computedMultiDep2';
+      const inlineSingleDep = 'inlineSingleDep';
+      const inlineMultiDep1 = 'inlineMultiDep1';
+      const inlineMultiDep2 = 'inlineMultiDep2';
 
       suite('check disabled', () => {
         test('no arguments defined', () => {
@@ -144,6 +210,36 @@
           el.multiProp2 = undefined;
           assertEffects({multiPropObserver: 3});
         });
+        test('computeSingle argument defined', () => {
+          setupElement(false, {computedSingleDep});
+          assertEffects({computeSingle: 1});
+          assert.equal(el.computedSingle, '[computedSingleDep]');
+        });
+        test('one computeMulti argument defined', () => {
+          setupElement(false, {computedMultiDep1});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.computedMulti, '[computedMultiDep1,undefined]');
+        });
+        test('all computeMulti argument defined', () => {
+          setupElement(false, {computedMultiDep1, computedMultiDep2});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.computedMulti, '[computedMultiDep1,computedMultiDep2]');
+        });
+        test('inline computeSingle argument defined', () => {
+          setupElement(false, {inlineSingleDep});
+          assertEffects({computeSingle: 1});
+          assert.equal(el.$.child.computedSingle, '[inlineSingleDep]');
+        });
+        test('one inline computeMulti argument defined', () => {
+          setupElement(false, {inlineMultiDep1});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,undefined]');
+        });
+        test('all inline computeMulti argument defined', () => {
+          setupElement(false, {inlineMultiDep1, inlineMultiDep2});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,inlineMultiDep2]');
+        });
       });
 
       suite('warn', () => {
@@ -183,6 +279,36 @@
           el.multiProp2 = undefined;
           assertEffects({multiPropObserver: 1, warn: 2});
         });
+        test('computeSingle argument defined', () => {
+          setupElement(true, {computedSingleDep});
+          assertEffects({computeSingle: 1});
+          assert.equal(el.computedSingle, '[computedSingleDep]');
+        });
+        test('one computeMulti argument defined', () => {
+          setupElement(true, {computedMultiDep1});
+          assertEffects({warn: 1});
+          assert.equal(el.computedMulti, undefined);
+        });
+        test('all computeMulti argument defined', () => {
+          setupElement(true, {computedMultiDep1, computedMultiDep2});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.computedMulti, '[computedMultiDep1,computedMultiDep2]');
+        });
+        test('inline computeSingle argument defined', () => {
+          setupElement(true, {inlineSingleDep});
+          assertEffects({computeSingle: 1});
+          assert.equal(el.$.child.computedSingle, '[inlineSingleDep]');
+        });
+        test('one inline computeMulti argument defined', () => {
+          setupElement(true, {inlineMultiDep1});
+          assertEffects({warn: 1});
+          assert.equal(el.$.child.computedMulti, undefined);
+        });
+        test('all inline computeMulti argument defined', () => {
+          setupElement(true, {inlineMultiDep1, inlineMultiDep2});
+          assertEffects({computeMulti: 1});
+          assert.equal(el.$.child.computedMulti, '[inlineMultiDep1,inlineMultiDep2]');
+        });
       });
     });
 
@@ -209,6 +335,34 @@
           el = fixture('declarative-multi-all');
           assertEffects({multiPropObserver: 1});
         });
+        test('computeSingle argument defined', () => {
+          el = fixture('declarative-single-computed');
+          assertEffects({computeSingle: 1});
+          assert.equal(el.computedSingle, '[a]');
+        });
+        test('one computeMulti arguments defined', () => {
+          el = fixture('declarative-multi-one-computed');
+          assertEffects({computeMulti: 0, warn: 1});
+          assert.equal(el.computedMulti, undefined);
+        });
+        test('all computeMulti defined', () => {
+          el = fixture('declarative-multi-all-computed');
+          assert.equal(el.computedMulti, '[b,c]');
+        });
+        test('inline computeSingle argument defined', () => {
+          el = fixture('declarative-single-computed-inline');
+          assertEffects({computeSingle: 1});
+          assert.equal(el.$.child.computedSingle, '[a]');
+        });
+        test('inline one computeMulti arguments defined', () => {
+          el = fixture('declarative-multi-one-computed-inline');
+          assertEffects({computeMulti: 0, warn: 1});
+          assert.equal(el.$.child.computedMulti, undefined);
+        });
+        test('inline all computeMulti defined', () => {
+          el = fixture('declarative-multi-all-computed-inline');
+          assert.equal(el.$.child.computedMulti, '[b,c]');
+        });
       });
     });