Address review comments

Author: rictic

README.md

@@ -13,3 +13,11 @@ This allows HTML with shadow roots to be serialized to plain HTML, and the seria
 `<template shadowroot>` elements are transformed bottom up so that in the case where they're nested, all elements within the declarative shadow tree stay inert until they have all been moved from their `<template shadowroot>` elements.
 
 If native support for `<template shadowroot>` is present, `hydrateShadowRoots` does nothing when called.
+
+### Known limitations
+
+* Does not currently look into imperatively created shadow roots.
+* The mutation observer implementation
+  * May not work properly in the face of streaming HTML parsing. Needs investigation.
+  * Will not notice imperatively created `<template shadowroot>` elements inside of other shadow roots.
+

bench/many_deep.html

@@ -8,14 +8,16 @@
     import { hydrateShadowRoots as manualWalkHydration } from '../_implementation/manual_walk.js';
     import { hydrateShadowRoots as querySelectorHydration } from '../_implementation/queryselectorall.js';
 
+    const implausiblyLongTimeToIndicateFailure = 9 * 1000 * 1000;
+
     function benchmark(hydrateMethod) {
       let hydrateShadowRoots;
       if (hydrateMethod === 'manualWalk') {
         hydrateShadowRoots = manualWalkHydration;
       } else if (hydrateMethod === 'querySelector') {
         hydrateShadowRoots = querySelectorHydration;
       } else {
-        return 9 * 1000 * 1000;
+        return implausiblyLongTimeToIndicateFailure;
       }
       let html = '';
       const n = 1000;
@@ -40,7 +42,7 @@
         return duration;
       } else {
         // hydration failed, still see templates in the document!
-        return 9 * 1000 * 1000;
+        return implausiblyLongTimeToIndicateFailure;
       }
     }
 

bench/many_flat.html

@@ -8,14 +8,16 @@
     import { hydrateShadowRoots as manualWalkHydration } from '../_implementation/manual_walk.js';
     import { hydrateShadowRoots as querySelectorHydration } from '../_implementation/queryselectorall.js';
 
+    const implausiblyLongTimeToIndicateFailure = 9 * 1000 * 1000;
+
     function benchmark(hydrateMethod) {
       let hydrateShadowRoots;
       if (hydrateMethod === 'manualWalk') {
         hydrateShadowRoots = manualWalkHydration;
       } else if (hydrateMethod === 'querySelector') {
         hydrateShadowRoots = querySelectorHydration;
       } else {
-        return 9 * 1000 * 1000;
+        return implausiblyLongTimeToIndicateFailure;
       }
       const html = `
         <div>
@@ -33,7 +35,7 @@
         return duration;
       } else {
         // hydration failed, still see templates in the document!
-        return 9 * 1000 * 1000;
+        return implausiblyLongTimeToIndicateFailure;
       }
     }
 

src/_implementation/manual_walk.ts

@@ -12,8 +12,8 @@
  * http://polymer.github.io/PATENTS.txt
  */
 
-import { hasNativeDeclarativeShadowRoots } from './feature_detect.js';
-import { isTemplate, isElement, hasNoParentElement } from './util.js';
+import {hasNativeDeclarativeShadowRoots} from './feature_detect.js';
+import {hasNoParentElement, isElement, isTemplate} from './util.js';
 
 /*
  * Traverses the DOM to find all <template> elements with a `shadowroot`
@@ -75,8 +75,8 @@ export const hydrateShadowRoots = (root: ParentNode) => {
               const shadow = host.attachShadow({mode, delegatesFocus});
               shadow.append(template.content);
             } catch {
-              // there was already a closed shadow root, so do nothing, and
-              // don't delete the template
+              // there was already a shadow root.
+              // TODO(rictic): log an error event?
             }
           } else {
             template = undefined;

src/_implementation/mutation_observer.ts

@@ -20,10 +20,13 @@ export function transformShadowRoots(within: Node = document) {
   if (hasNativeDeclarativeShadowRoots()) {
     return {mutationObserver: undefined, cleanup() {}};  // do nothing
   }
-  debugger;
   const mutationObserver = new MutationObserver((mutations, _) => {
-    debugger;
     for (const mutation of mutations) {
+      // TODO(rictic): test with streamed HTML that pauses in the middle
+      //     of a <template shadowroot> element. Do we get a mutation
+      //     with only part of the template's contents? What happens when
+      //     we remove the template from the DOM?
+      //     Tricky!
       hydrateShadowRoots(mutation.target as unknown as ParentNode);
     }
   });

src/_implementation/queryselectorall.ts

@@ -14,6 +14,22 @@
 
 import {hasNativeDeclarativeShadowRoots} from './feature_detect.js';
 
+interface StackEntry {
+  /**
+   * The nearest ancestor <template> element, if any, of the templates.
+   */
+  template: HTMLTemplateElement|undefined;
+  /**
+   * A stack of template elements inside `template` – or `root` if `template` is
+   * undefined – to be processed.
+   *
+   * The array is in reverse document order, such that the elements at the end
+   * of the array should be processed first (so that we can use templates.pop()
+   * to get the next element to process).
+   */
+  templates: HTMLTemplateElement[];
+}
+
 /*
  * Traverses the DOM to find all <template> elements with a `shadowroot`
  * attribute and move their content into a ShadowRoot on their parent element.
@@ -26,8 +42,8 @@ export const hydrateShadowRoots = (root: Element|DocumentFragment) => {
   if (hasNativeDeclarativeShadowRoots()) {
     return;  // nothing to do
   }
-  const stack = [{
-    template: undefined as undefined | HTMLTemplateElement,
+  const stack: StackEntry[] = [{
+    template: undefined,
     templates: Array.from(root.querySelectorAll('template')).reverse()
   }];
   while (stack.length > 0) {
@@ -45,8 +61,8 @@ export const hydrateShadowRoots = (root: Element|DocumentFragment) => {
             const shadow = host.attachShadow({mode, delegatesFocus});
             shadow.append(template.content);
           } catch {
-            // there was already a closed shadow root, so do nothing, and
-            // don't delete the template
+            // there was already a shadow root.
+            // TODO(rictic): log an error event?
           }
           host.removeChild(template);
         }