From c2f31edaa5596f9ab6c7770aba9578170cb7b1c7 Mon Sep 17 00:00:00 2001
From: Kevin Schaaf <kschaaf@google.com>
Date: Wed, 19 Jun 2019 16:36:47 -0700
Subject: [PATCH] Refactor DomIf into separate subclasses.

---
 lib/elements/dom-if.js         | 324 ++++++++++++++++++++-------------
 lib/mixins/property-effects.js |   6 +-
 lib/utils/templatize.js        |   1 +
 3 files changed, 197 insertions(+), 134 deletions(-)

diff --git a/lib/elements/dom-if.js b/lib/elements/dom-if.js
index 8c09d91293..5b801856af 100644
--- a/lib/elements/dom-if.js
+++ b/lib/elements/dom-if.js
@@ -40,7 +40,7 @@ import { showHideChildren } from '../utils/templatize.js';
  * @summary Custom element that conditionally stamps and hides or removes
  *   template content based on a boolean flag.
  */
-export class DomIf extends PolymerElement {
+class DomIfBase extends PolymerElement {
 
   // Not needed to find template; can be removed once the analyzer
   // can find the tag name from customElements.define call
@@ -87,10 +87,7 @@ export class DomIf extends PolymerElement {
   constructor() {
     super();
     this.__renderDebouncer = null;
-    this.__syncProps = null;
-    this.__instance = null;
     this._lastIf = false;
-    this.__ctor = null;
     this.__hideTemplateChildren__ = false;
   }
 
@@ -145,6 +142,56 @@ export class DomIf extends PolymerElement {
     }
   }
 
+  __ensureTemplate() {
+    // When `removeNestedTemplates` is true, the "template" is the element
+    // itself, which has been given a `_templateInfo` property
+    let template = this._templateInfo ? this :
+      /** @type {HTMLTemplateElement} */(wrap(this).querySelector('template'));
+    if (!template) {
+      // Wait until childList changes and template should be there by then
+      let observer = new MutationObserver(() => {
+        if (wrap(this).querySelector('template')) {
+          observer.disconnect();
+          this.__render();
+        } else {
+          throw new Error('dom-if requires a <template> child');
+        }
+      });
+      observer.observe(this, {childList: true});
+      return false;
+    }
+    this.__template = template;
+    return true;
+  }
+
+  __ensureInstance() {
+    let parentNode = wrap(this).parentNode;
+    if (!this.__hasInstance()) {
+      // Guard against element being detached while render was queued
+      if (!parentNode) {
+        return false;
+      }
+      // Find the template (when false, there was no template yet)
+      if (!this.__template && !this.__ensureTemplate()) {
+        return false;
+      }
+      this.__createAndInsertInstance(parentNode);
+    } else {
+      // Move instance children if necessary
+      let children = this.__instanceChildren();
+      if (children && children.length) {
+        // Detect case where dom-if was re-attached in new position
+        let lastChild = wrap(this).previousSibling;
+        if (lastChild !== children[children.length-1]) {
+          for (let i=0, n; (i<children.length) && (n=children[i]); i++) {
+            wrap(parentNode).insertBefore(n, this);
+          }
+        }
+      }
+    }
+    return true;
+  }
+
   /**
    * Forces the element to render its content. Normally rendering is
    * asynchronous to a provoking change. This is done for efficiency so
@@ -163,13 +210,11 @@ export class DomIf extends PolymerElement {
         // No template found yet
         return;
       }
-      this._showHideChildren();
+      this.__syncHostProperties();
     } else if (this.restamp) {
       this.__teardownInstance();
     }
-    if (!this.restamp && this.__instance) {
-      this._showHideChildren();
-    }
+    this._showHideChildren();
     if (this.if != this._lastIf) {
       this.dispatchEvent(new CustomEvent('dom-change', {
         bubbles: true,
@@ -178,134 +223,60 @@ export class DomIf extends PolymerElement {
       this._lastIf = this.if;
     }
   }
+}
 
-  __ensureInstance() {
-    let parentNode = wrap(this).parentNode;
-    // Guard against element being detached while render was queued
-    if (parentNode) {
-      if (!this.__template) {
-        // When `removeNestedTemplates` is true, the "template" is the element
-        // itself, which has been given a `_templateInfo` property
-        let template = this._templateInfo ? this :
-          /** @type {HTMLTemplateElement} */(wrap(this).querySelector('template'));
-        if (!template) {
-          // Wait until childList changes and template should be there by then
-          let observer = new MutationObserver(() => {
-            if (wrap(this).querySelector('template')) {
-              observer.disconnect();
-              this.__render();
-            } else {
-              throw new Error('dom-if requires a <template> child');
-            }
-          });
-          observer.observe(this, {childList: true});
-          return false;
-        }
-        this.__template = template;
-        if (!fastDomIf) {
-          this.__ctor = templatize(template, this, {
-            // dom-if templatizer instances require `mutable: true`, as
-            // `__syncHostProperties` relies on that behavior to sync objects
-            mutableData: true,
-            /**
-             * @param {string} prop Property to forward
-             * @param {*} value Value of property
-             * @this {DomIf}
-             */
-            forwardHostProp: function(prop, value) {
-              if (this.__instance) {
-                if (this.if) {
-                  this.__instance.forwardHostProp(prop, value);
-                } else {
-                  // If we have an instance but are squelching host property
-                  // forwarding due to if being false, note the invalidated
-                  // properties so `__syncHostProperties` can sync them the next
-                  // time `if` becomes true
-                  this.__syncProps = this.__syncProps || Object.create(null);
-                  this.__syncProps[root(prop)] = true;
-                }
-              }
-            }
-          });
-        }
+class DomIfFast extends DomIfBase {
+
+  constructor() {
+    super();
+    this.__instance = null;
+    this.__invalidProps = null;
+    this.__squelchedRunEffects = null;
+  }
+
+  __hasInstance() {
+    return Boolean(this.__instance);
+  }
+
+  __instanceChildren() {
+    return this.__instance.templateInfo.childNodes;
+  }
+
+  __createAndInsertInstance(parentNode) {
+    const host = this.__dataHost || this;
+    if (strictTemplatePolicy) {
+      if (!this.__dataHost) {
+        throw new Error('strictTemplatePolicy: template owner not trusted');
       }
-      if (!this.__instance) {
-        if (fastDomIf) {
-          const host = this.__dataHost || this;
-          if (strictTemplatePolicy) {
-            if (!this.__dataHost) {
-              throw new Error('strictTemplatePolicy: template owner not trusted');
-            }
-          }
-          // Pre-bind and link the template into the effects system
-          const templateInfo = host._bindTemplate(this.__template, true);
-          // Install runEffects hook that prevents running property effects
-          // (and any nested template effects) when the `if` is false
-          templateInfo.runEffects = runEffects => {
-            if (this.if) {
-              runEffects();
-            } else {
-              this.__syncProps = runEffects;
-            }
-          };
-          // Stamp the template, and set its DocumentFragment to the "instance"
-          this.__instance = host._stampTemplate(this.__template, templateInfo);
-          wrap(parentNode).insertBefore(this.__instance, this);
-        } else {
-          this.__instance = new this.__ctor();
-          wrap(parentNode).insertBefore(this.__instance.root, this);
-        }
+    }
+    // Pre-bind and link the template into the effects system
+    const templateInfo = host._bindTemplate(this.__template, true);
+    // Install runEffects hook that prevents running property effects
+    // (and any nested template effects) when the `if` is false
+    templateInfo.runEffects = (runEffects, changedProps) => {
+      if (this.if) {
+        runEffects(changedProps);
       } else {
-        let c$ = fastDomIf ? this.__instance.templateInfo.childNodes :
-          this.__instance.children;
-        if (c$ && c$.length) {
-          // Detect case where dom-if was re-attached in new position
-          let lastChild = wrap(this).previousSibling;
-          if (lastChild !== c$[c$.length-1]) {
-            for (let i=0, n; (i<c$.length) && (n=c$[i]); i++) {
-              wrap(parentNode).insertBefore(n, this);
-            }
-          }
-        }
+        this.__invalidProps = Object.assign(this.__invalidProps || {}, changedProps);
+        this.__squelchedRunEffects = runEffects;
       }
-    }
-    return true;
+    };
+    // Stamp the template, and set its DocumentFragment to the "instance"
+    this.__instance = host._stampTemplate(this.__template, templateInfo);
+    wrap(parentNode).insertBefore(this.__instance, this);
   }
 
   __syncHostProperties() {
-    let props = this.__syncProps;
-    this.__syncProps = null;
-    if (props) {
-      if (fastDomIf) {
-        props();
-      } else {
-        for (let prop in props) {
-          this.__instance._setPendingProperty(prop, this.__dataHost[prop]);
-        }
-        this.__instance._flushProperties();
-      }
+    const runEffects = this.__squelchedRunEffects;
+    if (runEffects) {
+      runEffects(this.__invalidProps);
+      this.__syncProps = this.__squelchedRunEffects = null;
     }
   }
 
   __teardownInstance() {
     if (this.__instance) {
-      if (fastDomIf) {
-        this._removeBoundDom(this.__instance);
-      } else {
-        let c$ = this.__instance.children;
-        if (c$ && c$.length) {
-          // use first child parent, for case when dom-if may have been detached
-          let parent = wrap(c$[0]).parentNode;
-          // Instance children may be disconnected from parents when dom-if
-          // detaches if a tree was innerHTML'ed
-          if (parent) {
-            parent = wrap(parent);
-            for (let i=0, n; (i<c$.length) && (n=c$[i]); i++) {
-              parent.removeChild(n);
-            }
-          }
-        }
-      }
+      this._removeBoundDom(this.__instance);
       this.__syncProps = null;
       this.__instance = null;
     }
@@ -320,19 +291,110 @@ export class DomIf extends PolymerElement {
    * @suppress {visibility}
    */
   _showHideChildren() {
-    let hidden = this.__hideTemplateChildren__ || !this.if;
+    const hidden = this.__hideTemplateChildren__ || !this.if;
     if (this.__instance) {
-      if (fastDomIf) {
-        showHideChildren(hidden, this.__instance.templateInfo.childNodes);
-      } else {
-        this.__instance._showHideChildren(hidden);
+      showHideChildren(hidden, this.__instance.templateInfo.childNodes);
+    }
+  }
+}
+
+class DomIfLegacy extends DomIfBase {
+
+  constructor() {
+    super();
+    this.__ctor = null;
+    this.__instance = null;
+    this.__invalidProps = null;
+  }
+
+  __hasInstance() {
+    return Boolean(this.__instance);
+  }
+
+  __instanceChildren() {
+    return this.__instance.children;
+  }
+
+  __createAndInsertInstance(parentNode) {
+    // Ensure we have an instance constructor
+    if (!this.__ctor) {
+      this.__ctor = templatize(this.__template, this, {
+        // dom-if templatizer instances require `mutable: true`, as
+        // `__syncHostProperties` relies on that behavior to sync objects
+        mutableData: true,
+        /**
+         * @param {string} prop Property to forward
+         * @param {*} value Value of property
+         * @this {DomIf}
+         */
+        forwardHostProp: function(prop, value) {
+          if (this.__instance) {
+            if (this.if) {
+              this.__instance.forwardHostProp(prop, value);
+            } else {
+              // If we have an instance but are squelching host property
+              // forwarding due to if being false, note the invalidated
+              // properties so `__syncHostProperties` can sync them the next
+              // time `if` becomes true
+              this.__invalidProps = this.__invalidProps || Object.create(null);
+              this.__invalidProps[root(prop)] = true;
+            }
+          }
+        }
+      });
+    }
+    // Create and insert the instance
+    this.__instance = new this.__ctor();
+    wrap(parentNode).insertBefore(this.__instance.root, this);
+  }
+
+  __teardownInstance() {
+    if (this.__instance) {
+      let c$ = this.__instance.children;
+      if (c$ && c$.length) {
+        // use first child parent, for case when dom-if may have been detached
+        let parent = wrap(c$[0]).parentNode;
+        // Instance children may be disconnected from parents when dom-if
+        // detaches if a tree was innerHTML'ed
+        if (parent) {
+          parent = wrap(parent);
+          for (let i=0, n; (i<c$.length) && (n=c$[i]); i++) {
+            parent.removeChild(n);
+          }
+        }
       }
-      if (!hidden) {
-        this.__syncHostProperties();
+      this.__invalidProps = null;
+      this.__instance = null;
+    }
+  }
+
+  __syncHostProperties() {
+    let props = this.__invalidProps;
+    if (props) {
+      for (let prop in props) {
+        this.__instance._setPendingProperty(prop, this.__dataHost[prop]);
       }
+      this.__instance._flushProperties();
+      this.__invalidProps = null;
     }
   }
 
+  /**
+   * Shows or hides the template instance top level child elements. For
+   * text nodes, `textContent` is removed while "hidden" and replaced when
+   * "shown."
+   * @return {void}
+   * @protected
+   * @suppress {visibility}
+   */
+  _showHideChildren() {
+    const hidden = this.__hideTemplateChildren__ || !this.if;
+    if (this.__instance) {
+      this.__instance._showHideChildren(hidden);
+    }
+  }
 }
 
+export const DomIf = fastDomIf ? DomIfFast : DomIfLegacy;
+
 customElements.define(DomIf.is, DomIf);
diff --git a/lib/mixins/property-effects.js b/lib/mixins/property-effects.js
index bec51d4cec..7bb2d4174c 100644
--- a/lib/mixins/property-effects.js
+++ b/lib/mixins/property-effects.js
@@ -1978,7 +1978,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
     }
 
     _runEffectsForTemplate(templateInfo, changedProps, oldProps, hasPaths) {
-      const baseRunEffects = () => {
+      const baseRunEffects = (changedProps) => {
         runEffects(this, templateInfo.propertyEffects, changedProps, oldProps,
           hasPaths, templateInfo.nodeList);
         for (let info=templateInfo.firstChild; info; info=info.nextSibling) {
@@ -1986,9 +1986,9 @@ export const PropertyEffects = dedupingMixin(superClass => {
         }
       };
       if (templateInfo.runEffects) {
-        templateInfo.runEffects(baseRunEffects);
+        templateInfo.runEffects(baseRunEffects, changedProps);
       } else {
-        baseRunEffects();
+        baseRunEffects(changedProps);
       }
     }
 
diff --git a/lib/utils/templatize.js b/lib/utils/templatize.js
index ba4eb3e04e..2f075f0442 100644
--- a/lib/utils/templatize.js
+++ b/lib/utils/templatize.js
@@ -392,6 +392,7 @@ function addPropagateEffects(target, templateInfo, options, methodHost) {
       } else {
         // Create a cached subclass of the base custom element class onto which
         // to put the template-specific propagate effects
+        /** @private */
         klass = class TemplatizedTemplateExtension extends target.constructor {};
       }
       // Add template - >instances effects