Updates based on review.

Author: kevinpschaaf

lib/elements/dom-if.js

@@ -142,28 +142,55 @@ class DomIfBase extends PolymerElement {
     }
   }
 
+  /**
+   * Ensures a template has been assigned to `this.__template`.  If it has not
+   * yet been, it querySelectors for it in its children and if it does not yet
+   * exist (e.g. in parser-generated case), opens a mutation observer and 
+   * waits for it to appear (returns false if it has not yet been found,
+   * otherwise true).  In the `removeNestedTemplates` case, the "template" will
+   * be the `dom-if` element itself.
+   * 
+   * @return {boolean} True when a template has been found, false otherwise
+   */
   __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;
+    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;
     }
-    this.__template = template;
     return true;
   }
 
+  /**
+   * Ensures a an instance of the template has been created and inserted. This
+   * method may return false if the template has not yet been found or if
+   * there is no `parentNode` to insert the template into (in either case,
+   * connection or the template-finding mutation observer firing will queue
+   * another render, causing this method to be called again at a more
+   * appropriate time).
+   * 
+   * Subclasses should implement the following methods called here:
+   * - `__hasInstance`
+   * - `__createAndInsertInstance`
+   * - `__getInstanceNodes`
+   * 
+   * @return {boolean} True if the instance was created, false otherwise.
+   */
   __ensureInstance() {
     let parentNode = wrap(this).parentNode;
     if (!this.__hasInstance()) {
@@ -172,13 +199,13 @@ class DomIfBase extends PolymerElement {
         return false;
       }
       // Find the template (when false, there was no template yet)
-      if (!this.__template && !this.__ensureTemplate()) {
+      if (!this.__ensureTemplate()) {
         return false;
       }
       this.__createAndInsertInstance(parentNode);
     } else {
       // Move instance children if necessary
-      let children = this.__instanceChildren();
+      let children = this.__getInstanceNodes();
       if (children && children.length) {
         // Detect case where dom-if was re-attached in new position
         let lastChild = wrap(this).previousSibling;
@@ -198,12 +225,22 @@ class DomIfBase extends PolymerElement {
    * that multiple changes trigger only a single render. The render method
    * should be called if, for example, template rendering is required to
    * validate application state.
+   * 
    * @return {void}
    */
   render() {
     flush();
   }
 
+  /**
+   * Performs the key rendering steps:
+   * 1. Ensure a template instance has been stamped (when true)
+   * 2. Remove the template instance (when false and restamp:true)
+   * 3. Sync the hidden state of the instance nodes with the if/restamp state
+   * 4. Fires the `dom-change` event when necessary
+   * 
+   * @return {void}
+   */
   __render() {
     if (this.if) {
       if (!this.__ensureInstance()) {
@@ -224,6 +261,25 @@ class DomIfBase extends PolymerElement {
   }
 }
 
+/**
+ * The version of DomIf used when `fastDomIf` setting is in use, which is
+ * optimized for first-render (but adds a tax to all subsequent property updates
+ * on the host, whether they were used in a given `dom-if` or not).
+ *
+ * This implementation avoids use of `Templatizer`, which introduces a new scope
+ * (a non-element PropertyEffects instance), which is not strictly necessary
+ * since `dom-if` never introduces new properties to its scope (unlike
+ * `dom-repeat`). Taking advantage of this fact, the `dom-if` reaches up to its
+ * `__dataHost` and stamps the template directly from the host using the host's
+ * runtime `_stampTemplate` API, which binds the property effects of the
+ * template directly to the host. This both avoids the intermediary
+ * `Templatizer` instance, but also avoids the need to bind host properties to
+ * the `<template>` element and forward those into the template instance.
+ *
+ * In this version of `dom-if`, the `this.__instance` method is the
+ * `DocumentFragment` returned from `_stampTemplate`, which also serves as the
+ * handle for later removing it using the `_removeBoundDom` method.
+ */
 class DomIfFast extends DomIfBase {
 
   constructor() {
@@ -233,14 +289,34 @@ class DomIfFast extends DomIfBase {
     this.__squelchedRunEffects = null;
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   * 
+   * @return {boolean} True when an instance has been created.
+   */
   __hasInstance() {
     return Boolean(this.__instance);
   }
 
-  __instanceChildren() {
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   *
+   * @return {Array<Node>} Array of child nodes stamped from the template
+   * instance.
+   */
+  __getInstanceNodes() {
     return this.__instance.templateInfo.childNodes;
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   *
+   * Stamps the template by calling `_stampTemplate` on the `__dataHost` of this
+   * element and then inserts the resulting nodes into the given `parentNode`.
+   *
+   * @param {Node} parentNode The parent node to insert the instance into
+   * @return {void}
+   */
   __createAndInsertInstance(parentNode) {
     const host = this.__dataHost || this;
     if (strictTemplatePolicy) {
@@ -255,13 +331,24 @@ class DomIfFast extends DomIfBase {
     templateInfo.runEffects = (runEffects, changedProps) => {
       if (this.if) {
         const invalidProps = this.__invalidProps;
+        // Mix any props that changed while the `if` was false into `changedProps`
         if (invalidProps) {
+          // If there were properties received while the `if` was false, it is
+          // important to sync the hidden state with the element _first_, so that
+          // new bindings to e.g. `textContent` do not get stomped on by
+          // pre-hidden values if `_showHideChildren` were to be called later at
+          // 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.__squelchedRunEffects = this.__invalidProps = null;
           this._showHideChildren();
           changedProps = Object.assign(invalidProps, changedProps);
         }
         runEffects(changedProps);
       } else {
+        // Store off 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
         this.__invalidProps = Object.assign(this.__invalidProps || {}, changedProps);
         this.__squelchedRunEffects = runEffects;
       }
@@ -271,6 +358,11 @@ class DomIfFast extends DomIfBase {
     wrap(parentNode).insertBefore(this.__instance, this);
   }
 
+  /**
+   * Run effects for any properties that changed while the `if` was false.
+   * 
+   * @return {void}
+   */
   __syncHostProperties() {
     const props = this.__invalidProps;
     if (props) {
@@ -279,16 +371,25 @@ class DomIfFast extends DomIfBase {
     }
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   * 
+   * Remove the instance and any nodes it created.  Uses the `__dataHost`'s
+   * runtime `_removeBoundDom` method.
+   * 
+   * @return {void}
+   */
   __teardownInstance() {
+    const host = this.__dataHost || this;
     if (this.__instance) {
-      this._removeBoundDom(this.__instance);
+      host._removeBoundDom(this.__instance);
       this.__syncProps = null;
       this.__instance = null;
     }
   }
 
   /**
-   * Shows or hides the template instance top level child elements. For
+   * Shows or hides the template instance top level child nodes. For
    * text nodes, `textContent` is removed while "hidden" and replaced when
    * "shown."
    * @return {void}
@@ -307,6 +408,12 @@ class DomIfFast extends DomIfBase {
   }
 }
 
+/**
+ * The "legacy" implementation of `dom-if`, implemented using `Templatizer`.
+ * 
+ * In this version, `this.__instance` is the `TemplateInstance` returned
+ * from the templatized constructor.
+ */
 class DomIfLegacy extends DomIfBase {
 
   constructor() {
@@ -316,14 +423,35 @@ class DomIfLegacy extends DomIfBase {
     this.__invalidProps = null;
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   * 
+   * @return {boolean} True when an instance has been created.
+   */
   __hasInstance() {
     return Boolean(this.__instance);
   }
 
-  __instanceChildren() {
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   *
+   * @return {Array<Node>} Array of child nodes stamped from the template
+   * instance.
+   */
+  __getInstanceNodes() {
     return this.__instance.children;
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   *
+   * Stamps the template by creating a new instance of the templatized
+   * constructor (which is created lazily if it does not yet exist), and then
+   * inserts its resulting `root` doc fragment into the given `parentNode`.
+   *
+   * @param {Node} parentNode The parent node to insert the instance into
+   * @return {void}
+   */
   __createAndInsertInstance(parentNode) {
     // Ensure we have an instance constructor
     if (!this.__ctor) {
@@ -357,6 +485,13 @@ class DomIfLegacy extends DomIfBase {
     wrap(parentNode).insertBefore(this.__instance.root, this);
   }
 
+  /**
+   * Implementation of abstract API needed by DomIfBase.
+   * 
+   * Removes the instance and any nodes it created.
+   * 
+   * @return {void}
+   */
   __teardownInstance() {
     if (this.__instance) {
       let c$ = this.__instance.children;
@@ -377,6 +512,12 @@ class DomIfLegacy extends DomIfBase {
     }
   }
 
+  /**
+   * Forwards any properties that changed while the `if` was false into the
+   * template instance and flushes it.
+   * 
+   * @return {void}
+   */
   __syncHostProperties() {
     let props = this.__invalidProps;
     if (props) {

lib/mixins/property-effects.js

@@ -2681,7 +2681,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
      */
     _bindTemplate(template, instanceBinding) {
       let templateInfo = this.constructor._parseTemplate(template);
-      let wasPreBound = this.__templateInfo == templateInfo;
+      let wasPreBound = this.__preBoundTemplateInfo == templateInfo;
       // Optimization: since this is called twice for proto-bound templates,
       // don't attempt to recreate accessors if this template was pre-bound
       if (!wasPreBound) {
@@ -2694,9 +2694,14 @@ export const PropertyEffects = dedupingMixin(superClass => {
         // and link into list of templates if necessary
         templateInfo = /** @type {!TemplateInfo} */(Object.create(templateInfo));
         templateInfo.wasPreBound = wasPreBound;
-        if (wasPreBound || !this.__templateInfo) {
+        if (!this.__templateInfo) {
+          // Set the info to the root of the tree
           this.__templateInfo = templateInfo;
         } else {
+          // Append this template info onto the end of its parent template's
+          // nested template list; if this template was not nested in another
+          // template, use the root template (the first stamped one) as the
+          // parent
           const parent = templateInfo.parent || this.__templateInfo;
           const previous = parent.lastChild;
           parent.lastChild = templateInfo;
@@ -2708,7 +2713,7 @@ export const PropertyEffects = dedupingMixin(superClass => {
           }
         }
       } else {
-        this.__templateInfo = templateInfo;
+        this.__preBoundTemplateInfo = templateInfo;
       }
       return templateInfo;
     }
@@ -2751,6 +2756,9 @@ export const PropertyEffects = dedupingMixin(superClass => {
      * in the main element template.
      *
      * @param {!HTMLTemplateElement} template Template to stamp
+     * @param {Object=} templateInfo Optional bound template info associated
+     *   with the template to be stamped; if omitted the template will be
+     *   automatically bound.
      * @return {!StampedTemplate} Cloned template content
      * @override
      * @protected

lib/mixins/template-stamp.js

@@ -436,6 +436,9 @@ export const TemplateStamp = dedupingMixin(
      * is removed and stored in notes as well.
      *
      * @param {!HTMLTemplateElement} template Template to stamp
+     * @param {Object=} templateInfo Optional template info associated
+     *   with the template to be stamped; if omitted the template will be
+     *   automatically parsed.
      * @return {!StampedTemplate} Cloned template content
      * @override
      */