Bounds must be non-empty

packages/@glimmer/node/lib/node-dom-helper.ts

@@ -1,6 +1,7 @@
 import * as SimpleDOM from 'simple-dom';
 import { DOMTreeConstruction, Bounds, ConcreteBounds } from '@glimmer/runtime';
 import { Simple } from '@glimmer/interfaces';
+import { Option } from '@glimmer/util';
 
 export default class NodeDOMTreeConstruction extends DOMTreeConstruction {
   protected document!: SimpleDOM.Document; // Hides property on base class
@@ -11,16 +12,10 @@ export default class NodeDOMTreeConstruction extends DOMTreeConstruction {
   // override to prevent usage of `this.document` until after the constructor
   protected setupUselessElement() {}
 
-  insertHTMLBefore(parent: Simple.Element, reference: Simple.Node, html: string): Bounds {
-    let prev = reference ? reference.previousSibling : parent.lastChild;
-
+  insertHTMLBefore(parent: Simple.Element, reference: Option<Simple.Node>, html: string): Bounds {
     let raw = this.document.createRawHTMLSection(html);
     parent.insertBefore(raw, reference);
-
-    let first = prev ? prev.nextSibling : parent.firstChild;
-    let last = reference ? reference.previousSibling : parent.lastChild;
-
-    return new ConcreteBounds(parent, first, last);
+    return new ConcreteBounds(parent, raw, raw);
   }
 
   // override to avoid SVG detection/work when in node (this is not needed in SSR)

packages/@glimmer/runtime/index.ts

@@ -77,7 +77,6 @@ export {
   DOMChanges as IDOMChanges,
   DOMTreeConstruction,
   isWhitespace,
-  insertHTMLBefore,
 } from './lib/dom/helper';
 export { normalizeProperty } from './lib/dom/props';
 export {

packages/@glimmer/runtime/lib/bounds.ts

@@ -1,11 +1,11 @@
 import { Simple } from '@glimmer/interfaces';
-import { Option, Destroyable } from '@glimmer/util';
+import { Option, Destroyable, expect } from '@glimmer/util';
 
 export interface Bounds {
   // a method to future-proof for wormholing; may not be needed ultimately
   parentElement(): Simple.Element;
-  firstNode(): Option<Simple.Node>;
-  lastNode(): Option<Simple.Node>;
+  firstNode(): Simple.Node;
+  lastNode(): Simple.Node;
 }
 
 export class Cursor {
@@ -19,77 +19,75 @@ export interface DestroyableBounds extends Bounds, Destroyable {}
 export class ConcreteBounds implements Bounds {
   constructor(
     public parentNode: Simple.Element,
-    private first: Option<Simple.Node>,
-    private last: Option<Simple.Node>
+    private first: Simple.Node,
+    private last: Simple.Node
   ) {}
 
-  parentElement() {
+  parentElement(): Simple.Element {
     return this.parentNode;
   }
-  firstNode() {
+
+  firstNode(): Simple.Node {
     return this.first;
   }
-  lastNode() {
+
+  lastNode(): Simple.Node {
     return this.last;
   }
 }
 
 export class SingleNodeBounds implements Bounds {
   constructor(private parentNode: Simple.Element, private node: Simple.Node) {}
 
-  parentElement() {
+  parentElement(): Simple.Element {
     return this.parentNode;
   }
-  firstNode() {
+
+  firstNode(): Simple.Node {
     return this.node;
   }
-  lastNode() {
+
+  lastNode(): Simple.Node {
     return this.node;
   }
 }
 
-export function bounds(
-  parent: Simple.Element,
-  first: Simple.Node,
-  last: Simple.Node
-): ConcreteBounds {
-  return new ConcreteBounds(parent, first, last);
-}
-
-export function single(parent: Simple.Element, node: Simple.Node): SingleNodeBounds {
-  return new SingleNodeBounds(parent, node);
-}
-
-export function move(bounds: Bounds, reference: Option<Simple.Node>) {
+export function move(bounds: Bounds, reference: Option<Simple.Node>): Option<Simple.Node> {
   let parent = bounds.parentElement();
   let first = bounds.firstNode();
   let last = bounds.lastNode();
 
-  let node: Option<Simple.Node> = first;
+  let current: Simple.Node = first;
 
-  while (node) {
-    let next = node.nextSibling;
-    parent.insertBefore(node, reference);
-    if (node === last) return next;
-    node = next;
-  }
+  while (true) {
+    let next = current.nextSibling;
+
+    parent.insertBefore(current, reference);
 
-  return null;
+    if (current === last) {
+      return next;
+    }
+
+    current = expect(next, 'invalid bounds');
+  }
 }
 
 export function clear(bounds: Bounds): Option<Simple.Node> {
   let parent = bounds.parentElement();
   let first = bounds.firstNode();
   let last = bounds.lastNode();
 
-  let node: Option<Simple.Node> = first;
+  let current: Simple.Node = first;
 
-  while (node) {
-    let next = node.nextSibling;
-    parent.removeChild(node);
-    if (node === last) return next;
-    node = next;
-  }
+  while (true) {
+    let next = current.nextSibling;
+
+    parent.removeChild(current);
 
-  return null;
+    if (current === last) {
+      return next;
+    }
+
+    current = expect(next, 'invalid bounds');
+  }
 }

packages/@glimmer/runtime/lib/compat/svg-inner-html-fix.ts

@@ -1,6 +1,7 @@
-import { Bounds, ConcreteBounds } from '../bounds';
+import { Bounds } from '../bounds';
 import { moveNodesBefore, DOMOperations } from '../dom/helper';
-import { Option, unwrap } from '@glimmer/util';
+import { Option, unwrap, assert } from '@glimmer/util';
+import { Simple } from '@glimmer/interfaces';
 
 export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';
 export type SVG_NAMESPACE = typeof SVG_NAMESPACE;
@@ -30,41 +31,51 @@ export function applySVGInnerHTMLFix(
   let div = document.createElement('div');
 
   return class DOMChangesWithSVGInnerHTMLFix extends DOMClass {
-    insertHTMLBefore(parent: HTMLElement, nextSibling: Node, html: string): Bounds {
+    insertHTMLBefore(
+      parent: Simple.Element,
+      nextSibling: Option<Simple.Node>,
+      html: string
+    ): Bounds {
+      if (html === '') {
+        return super.insertHTMLBefore(parent, nextSibling, html);
+      }
+
       if (parent.namespaceURI !== svgNamespace) {
         return super.insertHTMLBefore(parent, nextSibling, html);
       }
 
-      return fixSVG(parent, div, html, nextSibling);
+      // TODO: why are these casts okay???
+      return fixSVG(parent as Element, div, html, nextSibling as Option<Node>);
     }
   };
 }
 
-function fixSVG(parent: Element, div: HTMLElement, html: string, reference: Node): Bounds {
+function fixSVG(parent: Element, div: HTMLElement, html: string, reference: Option<Node>): Bounds {
+  assert(html !== '', 'html cannot be empty');
+
   let source: Node;
 
   // This is important, because decendants of the <foreignObject> integration
   // point are parsed in the HTML namespace
   if (parent.tagName.toUpperCase() === 'FOREIGNOBJECT') {
     // IE, Edge: also do not correctly support using `innerHTML` on SVG
     // namespaced elements. So here a wrapper is used.
-    let wrappedHtml = '<svg><foreignObject>' + (html || '<!---->') + '</foreignObject></svg>';
+    let wrappedHtml = '<svg><foreignObject>' + html + '</foreignObject></svg>';
 
     div.innerHTML = wrappedHtml;
 
     source = div.firstChild!.firstChild!;
   } else {
     // IE, Edge: also do not correctly support using `innerHTML` on SVG
     // namespaced elements. So here a wrapper is used.
-    let wrappedHtml = '<svg>' + (html || '<!---->') + '</svg>';
+    let wrappedHtml = '<svg>' + html + '</svg>';
 
     div.innerHTML = wrappedHtml;
 
     source = div.firstChild!;
   }
 
-  let [first, last] = moveNodesBefore(source, parent, reference);
-  return new ConcreteBounds(parent, first, last);
+  return moveNodesBefore(source, parent, reference);
 }
 
 function shouldApplyFix(document: Document, svgNamespace: SVG_NAMESPACE) {

packages/@glimmer/runtime/lib/compat/text-node-merging-fix.ts

@@ -1,6 +1,7 @@
 import { Bounds } from '../bounds';
 import { DOMOperations } from '../dom/helper';
 import { Option } from '@glimmer/util';
+import { Simple } from '@glimmer/interfaces';
 
 // Patch:    Adjacent text node merging fix
 // Browsers: IE, Edge, Firefox w/o inspector open
@@ -32,10 +33,19 @@ export function applyTextNodeMergingFix(
       this.uselessComment = document.createComment('');
     }
 
-    insertHTMLBefore(parent: HTMLElement, nextSibling: Node, html: string): Bounds {
+    insertHTMLBefore(
+      parent: Simple.Element,
+      nextSibling: Option<Simple.Node>,
+      html: string
+    ): Bounds {
+      if (html === '') {
+        return super.insertHTMLBefore(parent, nextSibling, html);
+      }
+
       let didSetUselessComment = false;
 
       let nextPrevious = nextSibling ? nextSibling.previousSibling : parent.lastChild;
+
       if (nextPrevious && nextPrevious instanceof Text) {
         didSetUselessComment = true;
         parent.insertBefore(this.uselessComment, nextSibling);

packages/@glimmer/runtime/lib/dom/helper.ts

@@ -3,7 +3,7 @@ import { applySVGInnerHTMLFix } from '../compat/svg-inner-html-fix';
 import { applyTextNodeMergingFix } from '../compat/text-node-merging-fix';
 import { Simple } from '@glimmer/interfaces';
 
-import { Option } from '@glimmer/util';
+import { Option, expect } from '@glimmer/util';
 
 export const SVG_NAMESPACE = 'http://www.w3.org/2000/svg';
 
@@ -77,17 +77,22 @@ export function isWhitespace(string: string) {
 export function moveNodesBefore(
   source: Simple.Node,
   target: Simple.Element,
-  nextSibling: Simple.Node
-) {
-  let first = source.firstChild;
-  let last: Simple.Node | null = null;
-  let current = first;
+  nextSibling: Option<Simple.Node>
+): Bounds {
+  let first = expect(source.firstChild, 'source is empty');
+  let last: Simple.Node = first;
+  let current: Option<Simple.Node> = first;
+
   while (current) {
+    let next: Option<Simple.Node> = current.nextSibling;
+
+    target.insertBefore(current, nextSibling);
+
     last = current;
-    current = current.nextSibling;
-    target.insertBefore(last, nextSibling);
+    current = next;
   }
-  return [first, last];
+
+  return new ConcreteBounds(target, first, last);
 }
 
 export class DOMOperations {
@@ -134,10 +139,44 @@ export class DOMOperations {
 
   insertHTMLBefore(
     _parent: Simple.Element,
-    nextSibling: Option<Simple.Node>,
+    _nextSibling: Option<Simple.Node>,
     html: string
   ): Bounds {
-    return insertHTMLBefore(this.uselessElement, _parent, nextSibling, html);
+    if (html === '') {
+      let comment = this.createComment('');
+      _parent.insertBefore(comment, _nextSibling);
+      return new ConcreteBounds(_parent, comment, comment);
+    }
+
+    // TODO why are these casts okay???
+    let parent = _parent as Element;
+    let nextSibling = _nextSibling as Option<Node>;
+
+    let prev = nextSibling ? nextSibling.previousSibling : parent.lastChild;
+    let last: Simple.Node;
+
+    if (nextSibling === null) {
+      parent.insertAdjacentHTML('beforeend', html);
+      last = expect(parent.lastChild, 'bug in insertAdjacentHTML?');
+    } else if (nextSibling instanceof HTMLElement) {
+      nextSibling.insertAdjacentHTML('beforebegin', html);
+      last = expect(nextSibling.previousSibling, 'bug in insertAdjacentHTML?');
+    } else {
+      // Non-element nodes do not support insertAdjacentHTML, so add an
+      // element and call it on that element. Then remove the element.
+      //
+      // This also protects Edge, IE and Firefox w/o the inspector open
+      // from merging adjacent text nodes. See ./compat/text-node-merging-fix.ts
+      let { uselessElement } = this;
+
+      parent.insertBefore(uselessElement, nextSibling);
+      uselessElement.insertAdjacentHTML('beforebegin', html);
+      last = expect(uselessElement.previousSibling, 'bug in insertAdjacentHTML?');
+      parent.removeChild(uselessElement);
+    }
+
+    let first = expect(prev ? prev.nextSibling : parent.firstChild, 'bug in insertAdjacentHTML?');
+    return new ConcreteBounds(parent, first, last);
   }
 
   createTextNode(text: string): Simple.Text {
@@ -205,43 +244,6 @@ export class DOMChanges extends DOMOperations {
   }
 }
 
-export function insertHTMLBefore(
-  this: void,
-  useless: HTMLElement,
-  _parent: Simple.Element,
-  _nextSibling: Option<Simple.Node>,
-  _html: string
-): Bounds {
-  let parent = _parent as Element;
-  let nextSibling = _nextSibling as Option<Node>;
-
-  let prev = nextSibling ? nextSibling.previousSibling : parent.lastChild;
-  let last: Simple.Node | null;
-
-  let html = _html || '<!---->';
-
-  if (nextSibling === null) {
-    parent.insertAdjacentHTML('beforeend', html);
-    last = parent.lastChild;
-  } else if (nextSibling instanceof HTMLElement) {
-    nextSibling.insertAdjacentHTML('beforebegin', html);
-    last = nextSibling.previousSibling;
-  } else {
-    // Non-element nodes do not support insertAdjacentHTML, so add an
-    // element and call it on that element. Then remove the element.
-    //
-    // This also protects Edge, IE and Firefox w/o the inspector open
-    // from merging adjacent text nodes. See ./compat/text-node-merging-fix.ts
-    parent.insertBefore(useless, nextSibling);
-    useless.insertAdjacentHTML('beforebegin', html);
-    last = useless.previousSibling;
-    parent.removeChild(useless);
-  }
-
-  let first = prev ? prev.nextSibling : parent.firstChild;
-  return new ConcreteBounds(parent, first, last);
-}
-
 let helper = DOMChanges;
 
 helper = applyTextNodeMergingFix(doc, helper) as typeof DOMChanges;