Issue with webcomponents that have HTML Comment elements

[spion]

Some webcomponents (like Ionic's ion-label) generate additional comment children

For those, the code frragment

dom-expressions/packages/dom-expressions/src/client.js

Line 428 in 8d2d82f

current = parent.firstChild.data = value;

is going to try and update the comment rather than the actual text content.

A repro that isn't very minimal: https://stackblitz.com/edit/github-pyx5ga?file=package.json,app%2Fclient.tsx,vite.config.js,index.html,app%2Fionic-css.ts

You will notice that <IonLabel /> generates empty comment elements, which in turn cause SolidJS to update them if the only child of the component is a string

[ryansolid]

Our system works by walking known nodes, I don't don't know if there is anything to be done here. I'm open to suggestions but if something changes the shape of the HTML from what the JSX outputs we aren't going to know where things are.

[titoBouzout]

Yes, we can do something. The fatal flaw in insertExpression is the assumption that the node is "ours". Instead of assuming the node it's ours we can save in current the actual node(to make it actually "ours"). insertExpression shouldn't make any assumption, because doing so will break any code that changes the tree.

I am not trying to be far-fetched, what I mean is that as long as we have references to nodes, and as long as "others" do not mess up with "our" stuff(nodes), then we can keep doing our thing without breaking anything.

[titoBouzout]

Hope that was clear, let me know if wasnt

[mdynnl]

We can address this here along with similar cases. The necessary change also shouldn't have that much impact on perf as we just need to flip it to make the node "ours".

[titoBouzout]

That would be nice, I only identify these two lines that could become problematic. In theory, it should be a matter of saving in current the text nodes(creation needed), instead of the text, and then making sure it compares to that, I see in the multi case sort of already does. This function is a bit complex

dom-expressions/packages/dom-expressions/src/client.js

Lines 479 to 480 in a1d24a1

	current = parent.firstChild.data = value;
	} else current = parent.textContent = value;

[ryansolid]

Yeah performance is a big deal here. Otherwise everything would be multi. By assuming we render all children shortcuts a bunch of operations. That being said I believe this issue is different than the moving nodes in the issue.

[titoBouzout]

Just had an idea, the original code:

if (current !== "" && typeof current === "string")
  current = parent.firstChild.data = value;
else 
  current = parent.textContent = value;

maybe could become

if (current instanceof Text) 
  current.data = value;
else {
  if (parent.childNodes.length === 0) {
    parent.textContent = value;
    current = parent.firstChild 
  } else {
    current  = document.createTextNode(value)
    parent.appendChild(current)
  }
}

assuming

• the if and else of the original code are closely related
• that the parent.textContent was used to just avoid createTextNode and parend.appendChild
• the current !== '' was used as a way to know if parent.textContent = value was done before
• that the else case is not used as a trick for "clearing"
• changing current to a Text doesn't complicate the logic elsewhere (this I haven't verified)

performance:

This should in theory perform very similar, the current instanceof Text could maybe be improved with nodeType which I suspect will perform the same as the previous if.

I am not sure of the penalty of node.childNodes.length, but that's basically the only thing different with the previous code in that else case, well, beside accessing parent.firstChild hehe which shouldn't be that costly.

The penalty in createTextNode is irrelevant because that's the case where the code breaks, and we are trying to fix.

Do you have a way to measure this possible change? It may be worthy to not break third party code that slightly changes the tree.

[titoBouzout]

@mdynnl what do you think?

[mdynnl]

Yeah, this is what I had in mind with my initial understanding but something obvious was missing. Then I realized if we go this way, we would also have to handle other cases of current as well, which is what cleanChildren does essentially. I believe Ryan was mentioning this above.

This case is so common and would have a huge impact on perf 😆. Anything that renders a dynamic sole child gets basically deopts to the case where something comes after. (i.e <div>{count()}</div> vs <div>{count()}!</div>)

The latter is also how this could be solved from consuming side by appending non-empty text(e.g <div>{count()}{' '}</div>) or prepending empty text effectively marking this as multi. I'm not sure if it makes sense to provide some form of annotations like we do for @ once.

[titoBouzout]

If I understand correctly, the proposed code

• doesn't make of single a multi
• it keeps the happy path for updates as in node.data = value
• It's only maybe slightly slower on creation because of the childNodes.length check (with automatic deopt when childNodes.length>0 as something external inserted a node there, but also this creation deopt doesn't spill to updates as it keeps the happy path for updates.

It could possibly may be slower here, but we can check there if current is a text node. And this in theory shouldn't happen because a signal won't trigger an update to the same value.

dom-expressions/packages/dom-expressions/src/client.js

Line 460 in a1d24a1

if (value === current) return current;

In clearChildren.. seems like we can do the same childeNodes.length/current.nodeType check

dom-expressions/packages/dom-expressions/src/client.js

Line 574 in a1d24a1

if (marker === undefined) return (parent.textContent = "");

All of that, if I am not missing something, I am missing something? 🤔

[mdynnl]

Yeah, it's that current could also be an array of nodes from rendering fragment/array i.e <div>{show() ? 1 : [1, 2]}</div>. playground

For example, it would be like this and we'd also need to follow up in a couple of places as well.

if (current instanceof Text)
  current.data = value;
else {
  if (current && current.length) {
    for (let i = current.length - 1; i >= 0; i--) current[i].remove();
  }
  current = document.createTextNode(value)
  parent.appendChild(current)
}

Just for the idea, I'm going to open a PR for this so we have a place to point people to.

[titoBouzout]

Nice, thank you, I was about to open a PR too,

I am strictly speaking of this condition, it cannot be an array there

    } else {
      if (current !== "" && typeof current === "string") {
        current = parent.firstChild.data = value;
      } else current = parent.textContent = value;
    }

[titoBouzout]

OK, so just for documenting here, mdynnl explained to me, that the issue is when current morphs between for example a string value to an array value (or in reverse), as described with their

<div>{show() ? 1 : [1, 2]}</div>

I missed that when they first explained it.

Now understanding that, some assumptions go out of the window, but also this isn't a fairly common case. So maybe we can do something about it. @mdynnl will be working on a PR.

One could probably make the case that if you need to touch the dom then wrap your things, but that's heavily annoying. I think as long as we can keep the happy path mostly untouched or not impacted in considerable ways, then fixing this will be super welcome for when solid is used with third party stuff. Like in there's no reason for this to happen other than performance.

[mdynnl]

#337 (comment)

Yeah performance is a big deal here. Otherwise everything would be multi. By assuming we render all children shortcuts a bunch of operations. That being said I believe this issue is different than the moving nodes in the issue.

Ok, this weekend, I think I finally got around to the scope of what Ryan said. This assumption that we render all children is for these cases specifically as a fast path, as described in #337 (comment).

I still think we should at least give a way to opt out this though e.g /*@multi*/, besides mentioning this in the documentation.

The opt out does exist today by appending empty text node before/after the children right under a native(or custom) element. https://playground.solidjs.com/anonymous/4933161a-2c9c-4abd-b70f-43e44a22f009

For ionic solid, that would be https://github.com/ionic-solidjs/ionic-solidjs/blob/791995e07ff93702cdd1fde5989194e561ef2d9c/packages/core/src/components/generated/ion-accordion.tsx#L26 for accordion.

[mdynnl]

This is what I've got so far and it does pass this issue and a few other cases too, but continuing this basically leads to implementing the same logic as multi case.

diff --git a/packages/dom-expressions/src/client.js b/packages/dom-expressions/src/client.js
index e4e6460a..633d70ac 100644
--- a/packages/dom-expressions/src/client.js
+++ b/packages/dom-expressions/src/client.js
@@ -475,13 +475,26 @@ function insertExpression(parent, value, current, marker, unwrapArray) {
       } else node = document.createTextNode(value);
       current = cleanChildren(parent, current, marker, node);
     } else {
-      if (current !== "" && typeof current === "string") {
-        current = parent.firstChild.data = value;
-      } else current = parent.textContent = value;
+      if (current && Array.isArray(current) && current.length) {
+        for (let i = current.length - 1; i--;) current[i].remove();
+        current = current[0];
+      }
+      if (current && current.nodeType === 3) {
+        current.data !== value && (current.data = value)
+      } else {
+        current && current.remove();
+        current = document.createTextNode(value);
+        parent.appendChild(current);
+      }
     }
   } else if (value == null || t === "boolean") {
     if (hydrating) return current;
-    current = cleanChildren(parent, current, marker);
+    if (multi) {
+      current = cleanChildren(parent, current, marker);
+    } else {
+      current && current.remove()
+      current = value
+    }
   } else if (t === "function") {
     effect(() => {
       let v = value();
@@ -513,8 +526,8 @@ function insertExpression(parent, value, current, marker, unwrapArray) {
         appendNodes(parent, array, marker);
       } else reconcileArrays(parent, current, array);
     } else {
-      current && cleanChildren(parent);
-      appendNodes(parent, array);
+      appendNodes(parent, array, current);
+      current && current.remove();
     }
     current = array;
   } else if (value.nodeType) {
@@ -524,7 +537,7 @@ function insertExpression(parent, value, current, marker, unwrapArray) {
       cleanChildren(parent, current, null, value);
     } else if (current == null || current === "" || !parent.firstChild) {
       parent.appendChild(value);
-    } else parent.replaceChild(value, parent.firstChild);
+    } else parent.replaceChild(value, current);
     current = value;
   } else if ("_DX_DEV_") console.warn(`Unrecognized value. Skipped inserting`, value);

[titoBouzout]

@mdynnl can you send it as a pr please, would be nice to fix this.

I kind of suspect we can still keep the happy path, for example this

      } else {
        current && current.remove();
        current = document.createTextNode(value);
        parent.appendChild(current);
      }

becoming to somewhat the equivalent of what we had before

      } else {
        if( parent.childNodes.length < 2 ){
           parent.textContent = value
           current  = parent.firstChild
        } else {
          current && current.remove();
          current = document.createTextNode(value);
          parent.appendChild(current);
        }
      }