Store syncInfo on instance and don't sync paths. Fixes #5629

* Previously the `syncInfo` used to store changed properties while the dom-if was false was stored on the dom-if. This means that even under `restamp`, any stored properties from a previous instance would be applied to the next instance. This change moves the `syncInfo` storage to the instance, so it naturally goes away when the instance is discarded.  Any new instance will take current values from the host.
* Due to limitations described in #4818, it is bad/illegal to allow paths to be batched and played through `runEffects`, since effect de-duping occurs by _root property_, so effects will only be running once for the first path matching an effect, with all other paths being dropped on the ground.  This can be particularly bad if the user e.g. `set` a path to an object, and then subsequently nulled the object; the observer would then be acting on a path that is no longer valid.  This change only stores the root property & value for any paths that come in, which matches the non-`fastDomIf` behavior with only storing `root(prop)` in `__invalidProps`.

lib/elements/dom-if.js

@@ -252,7 +252,7 @@ class DomIfBase extends PolymerElement {
       this.__teardownInstance();
     }
     this._showHideChildren();
-    if ((!suppressTemplateNotifications || this.notifyDomChange) 
+    if ((!suppressTemplateNotifications || this.notifyDomChange)
         && this.if != this._lastIf) {
       this.dispatchEvent(new CustomEvent('dom-change', {
         bubbles: true,
@@ -339,7 +339,6 @@ class DomIfFast extends DomIfBase {
   constructor() {
     super();
     this.__instance = null;
-    this.__syncInfo = null;
   }
 
   /**
@@ -386,7 +385,8 @@ class DomIfFast extends DomIfBase {
     // Install runEffects hook that prevents running property effects
     // (and any nested template effects) when the `if` is false
     templateInfo.runEffects = (runEffects, changedProps, hasPaths) => {
-      const syncInfo = this.__syncInfo;
+      const instance = this.__instance;
+      let syncInfo = instance && instance.__syncInfo;
       if (this.if) {
         // Mix any props that changed while the `if` was false into `changedProps`
         if (syncInfo) {
@@ -397,25 +397,34 @@ class DomIfFast extends DomIfBase {
           // the next render. Clearing `__invalidProps` here ensures
           // `_showHideChildren`'s call to `__syncHostProperties` no-ops, so
           // that we don't call `runEffects` more often than necessary.
-          this.__syncInfo = null;
+          instance.__syncInfo = null;
           this._showHideChildren();
           changedProps = Object.assign(syncInfo.changedProps, changedProps);
-          hasPaths = hasPaths || syncInfo.hasPaths;
         }
         runEffects(changedProps, hasPaths);
       } else {
         // Accumulate any values changed while `if` was false, along with the
         // runEffects method to sync them, so that we can replay them once `if`
         // becomes true
-        if (syncInfo) {
-          syncInfo.hasPaths = syncInfo.hasPaths || hasPaths;
-          Object.assign(syncInfo.changedProps, changedProps);
-        } else {
-          this.__syncInfo = {
-            runEffects,
-            changedProps: Object.assign({}, changedProps),
-            hasPaths
-          };
+        if (instance) {
+          if (!syncInfo) {
+            syncInfo = instance.__syncInfo = { runEffects, changedProps: {} };
+          }
+          if (hasPaths) {
+            // Store root object of any paths; this will ensure direct bindings
+            // like [[obj.foo]] bindings run after a `set('obj.foo', v)`, but
+            // note that path notifications like `set('obj.foo.bar', v)` will
+            // not propagate. Since batched path notifications are not
+            // supported, we cannot simply accumulate path notifications. This
+            // is equivalent to the non-fastDomIf case, which stores root(p) in
+            // __invalidProps.
+            for (const p in changedProps) {
+              const rootProp = root(p);
+              syncInfo.changedProps[rootProp] = this.__dataHost[rootProp];
+            }
+          } else {
+            Object.assign(syncInfo.changedProps, changedProps);
+          }
         }
       }
     };
@@ -431,10 +440,11 @@ class DomIfFast extends DomIfBase {
    * @return {void}
    */
   __syncHostProperties() {
-    const syncInfo = this.__syncInfo;
+    const instance = this.__instance;
+    const syncInfo = instance && instance.__syncInfo;
     if (syncInfo) {
-      this.__syncInfo = null;
-      syncInfo.runEffects(syncInfo.changedProps, syncInfo.hasPaths);
+      instance.__syncInfo = null;
+      syncInfo.runEffects(syncInfo.changedProps, false);
     }
   }
 

test/unit/dom-if-elements.js

@@ -321,16 +321,18 @@ Polymer({
       observer: 'propChanged'
     }
   },
+  observers: ['pathChanged(obj.*)'],
   created() {
     this.propChanged = sinon.spy();
+    this.pathChanged = sinon.spy();
   }
 });
 
 Polymer({
   _template: html`
     <template is="dom-if" if="{{b}}" restamp="{{restamp}}">
       {{guarded(a)}}
-      <prop-observer id="observer" prop="[[c.d]]"></prop-observer>
+      <prop-observer id="observer" prop="[[c.d]]" obj="[[c]]"></prop-observer>
     </template>
 `,
 

test/unit/dom-if.html

@@ -940,21 +940,33 @@
       el.a = 'ok';
       el.c = {d: 'ok'};
       assert.equal(el.shadowRoot.textContent.trim(), 'ok');
-      assert.equal(el.shadowRoot.querySelector('#observer').propChanged.callCount, 1);
+      let observer = el.shadowRoot.querySelector('#observer');
+      assert.equal(observer.propChanged.callCount, 1);
+      assert.equal(observer.pathChanged.callCount, 1);
+      assert.equal(observer.pathChanged.getCalls()[0].args[0].path, 'obj');
       el.b = false;
       el.a = 'notok';
       el.set('c.d', 'notok');
       flush();
       assert.equal(el.shadowRoot.textContent.trim(), '');
       if (!restamp) {
-        assert.equal(el.shadowRoot.querySelector('#observer').propChanged.callCount, 1);
+        const observer = el.shadowRoot.querySelector('#observer');
+        assert.equal(observer.propChanged.callCount, 1);
+        assert.equal(observer.pathChanged.callCount, 1);
       }
       el.set('c.d', 'changed');
       el.a = 'changed';
       el.b = true;
       flush();
       assert.equal(el.shadowRoot.textContent.trim(), 'changed');
-      assert.equal(el.shadowRoot.querySelector('#observer').propChanged.callCount, restamp ? 1 : 2);
+      observer = el.shadowRoot.querySelector('#observer');
+      // Note that path bindings in a falsey dom-if will be updated when the
+      // dom-if becomes truthy, however path notifications to other elements
+      // will be lost; this is a fundamental limitation due to the fact that
+      // batched path notifications are not supported. Hence the `propChanged`
+      // count increments, but the `pathChanged` count does not.
+      assert.equal(observer.propChanged.callCount, restamp ? 1 : 2);
+      assert.equal(observer.pathChanged.callCount, 1);
       document.body.removeChild(el);
     });