From ed6deea83b4206560e9bad56dc30f2257de02b73 Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Wed, 31 Oct 2018 10:16:54 -0700
Subject: [PATCH] Sync with changes made on master.

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

diff --git a/lib/legacy/legacy-data-mixin.html b/lib/legacy/legacy-data-mixin.html
index eef8802a57..0d0804b097 100644
--- a/lib/legacy/legacy-data-mixin.html
+++ b/lib/legacy/legacy-data-mixin.html
@@ -9,6 +9,7 @@
 -->
 
 <link rel="import" href="class.html">
+<link rel="import" href="../polymer.html">
 <link rel="import" href="../utils/mixin.html">
 
 <script>
@@ -16,15 +17,43 @@
   'use strict';
 
   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
@@ -38,6 +67,11 @@
    * 2.x+ by allowing legacy code to work while identifying observers and
    * computing functions that need undefined checks to work without
    * the mixin in Polymer 2.
+   *
+   * @mixinFunction
+   * @polymer
+   * @summary Mixin to selectively add back Polymer 1.x's `undefined` rules
+   * governing when observers & computing functions run.
    */
   Polymer.LegacyDataMixin = Polymer.dedupingMixin(superClass => {
 
@@ -59,12 +93,16 @@
        */
       _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);
             }
           }
         }
@@ -81,22 +119,23 @@
        * @return {void}
        * @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);
+      _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));
       }
 
     }
@@ -105,6 +144,9 @@
 
   });
 
+  // LegacyDataMixin is applied to base class _before_ metaprogramming, to
+  // ensure override of _addPropertyEffect et.al. are used by metaprogramming
+  // performed in _finalizeClass
   const Class = Polymer.Class;
   Polymer.Class = (info, mixin) => Class(info, 
     superClass => mixin ? 
diff --git a/test/unit/legacy-data.html b/test/unit/legacy-data.html
index 4489142c52..4da340237b 100644
--- a/test/unit/legacy-data.html
+++ b/test/unit/legacy-data.html
@@ -19,16 +19,34 @@
 <body>
 
   <dom-module id="x-data">
+    <template>
+      <div id="child"
+        computed-single="[[computeSingle(inlineSingleDep)]]"
+        computed-multi="[[computeMulti(inlineMultiDep1, inlineMultiDep2)]]">
+      </div>
+    </template>
     <script>
       HTMLImports.whenReady(() => {
         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)',
@@ -39,6 +57,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]');
+        });
       });
     });