Fix TypeError when morphing a text or comment node via outerHTML

[lizarusi]

Hi!

While using idiomorph at Walnut, we hit a bug that we've been carrying as a local patch — contributing the fix upstream.

The bug

Morphing a text or comment node with morphStyle: "outerHTML" throws:

TypeError: root.querySelectorAll is not a function
    at findIdElements (src/idiomorph.js:1088)
    at createIdMaps (src/idiomorph.js:1143)
    at createMorphContext (src/idiomorph.js:1003)
    at Object.morph (src/idiomorph.js:159)

Repro — e.g. replacing a comment placeholder with rendered content:

const parent = document.createElement("div");
parent.innerHTML = "<p>Before</p><!-- placeholder --><p>After</p>";
Idiomorph.morph(parent.childNodes[1], "<button>Bar</button>", {
  morphStyle: "outerHTML",
}); // 💥 TypeError: root.querySelectorAll is not a function

morph() passes the raw oldNode into createIdMaps → findIdElements, which calls root.querySelectorAll("[id]") — but text and comment nodes don't have querySelectorAll. We hit this in production at Walnut (morphing captured DOM that includes bare text/comment nodes) and have been carrying this fix as a local patch since 0.7.2.

The fix

Guard the call with optional chaining — the exact pattern this function already uses one line below for getAttribute ("root could be a document fragment which doesn't have getAttribute"). With the guard in place the rest of the algorithm handles non-element nodes correctly: the new tests show a text node and a comment node being morphed into the expected content, with siblings preserved.

First commit adds the failing tests, second commit makes them pass. npm run typecheck, npm run format:check, and the full suite pass, with coverage at 100%.

[myabc]

Nice, minimal fix - and I like that the guard mirrors the getAttribute?. line right underneath it.

One gap though: the types don't follow the runtime. morph() is still @param {Element | Document} oldNode, so the repro from your description doesn't actually compile against the published dist/idiomorph.d.ts:

error TS2345: Argument of type 'ChildNode' is not assignable to parameter of type 'Element | Document'.

Same story one level down - findIdElements is now explicitly documented and guarded to accept text/comment/fragment roots, but still says @param {Element} root.

I had a go at widening it. The bit that surprised me: you can't just widen the JSDoc, because oldNode = normalizeElement(oldNode) reassigns the parameter and TS then drops that narrowing inside the saveAndRestoreFocus closure - 4 errors. Assigning to a fresh const instead clears all 4 with no casts, and only the two places that genuinely leave the Element world need one each. {Node} rather than a union, since that's already what the file uses for oldNode elsewhere (e.g. lines 457, 481, 629, 671):

@@ -85,3 +85,3 @@
  *
- * @param {Element | Document} oldNode
+ * @param {Node} oldNode
  * @param {Element | Node | HTMLCollection | Node[] | string | null} newContent
@@ -150,3 +150,3 @@ var Idiomorph = (function () {
    *
-   * @param {Element | Document} oldNode
+   * @param {Node} oldNode
    * @param {Element | Node | HTMLCollection | Node[] | string | null} newContent
@@ -156,5 +156,5 @@ var Idiomorph = (function () {
   function morph(oldNode, newContent, config = {}) {
-    oldNode = normalizeElement(oldNode);
+    const oldElement = normalizeElement(oldNode);
     const newNode = normalizeParent(newContent);
-    const ctx = createMorphContext(oldNode, newNode, config);
+    const ctx = createMorphContext(oldElement, newNode, config);

@@ -163,3 +163,3 @@ var Idiomorph = (function () {
         ctx,
-        oldNode,
+        oldElement,
         newNode,
@@ -167,6 +167,6 @@ var Idiomorph = (function () {
           if (ctx.morphStyle === "innerHTML") {
-            morphChildren(ctx, oldNode, newNode);
-            return Array.from(oldNode.childNodes);
+            morphChildren(ctx, oldElement, newNode);
+            return Array.from(oldElement.childNodes);
           } else {
-            return morphOuterHTML(ctx, oldNode, newNode);
+            return morphOuterHTML(ctx, oldElement, newNode);
           }
@@ -1086,5 +1086,5 @@ var Idiomorph = (function () {
     /**
-     * Returns all elements with an ID contained within the root element and its descendants
+     * Returns all elements with an ID contained within the root node and its descendants
      *
-     * @param {Element} root
+     * @param {Node} root
      * @returns {Element[]}
@@ -1092,7 +1092,8 @@ var Idiomorph = (function () {
     function findIdElements(root) {
-      // root could be a text or comment node which doesn't have `querySelectorAll`
-      let elements = Array.from(root.querySelectorAll?.("[id]") ?? []);
-      // root could be a document fragment which doesn't have `getAttribute`
-      if (root.getAttribute?.("id")) {
-        elements.push(root);
+      // root could be a text or comment node which doesn't have `querySelectorAll`,
+      // or a document fragment which doesn't have `getAttribute`
+      const elt = /** @type {Partial<Element>} */ (root);
+      let elements = Array.from(elt.querySelectorAll?.("[id]") ?? []);
+      if (elt.getAttribute?.("id")) {
+        elements.push(/** @type {Element} */ (root));
       }
@@ -1218,3 +1219,3 @@ var Idiomorph = (function () {
      *
-     * @param {Element | Document} content
+     * @param {Node} content
      * @returns {Element}
@@ -1225,3 +1226,5 @@ var Idiomorph = (function () {
       } else {
-        return content;
+        // content may be a text or comment node, which has no Element API;
+        // the algorithm only ever treats it as an opaque node to be replaced
+        return /** @type {Element} */ (content);
       }

On top of current main: tsc clean, format:check clean, 184 passed / 0 failed, coverage 100%, and the repro above compiles.

This overlaps #103, so it might be a call for the maintainers rather than something to add to this PR. This is a drive-by suggestion, as I am not a maintainer.

[botandrose]

Thank you @lizarusi for the PR, and to @myabc for the review!

To save you from another rebase, I did one myself. I also added another commit taking @myabc's suggestion of widening oldNode to {Node}, which is a move I've wanted to make, anyways.

The tradeoff here is that we have traded a compile-time error for a runtime error when oldNode is { Text | Comment } and an innerHTML morph is attempted, which I think is acceptable.