[rdom] comment nodes cause exception

[usernolan]

This may be ill-advised, but I was hoping that I could add comments to the DOM from rdom. Long story short: special content and exposition for those who care to inspect.

From hiccup: https://github.com/thi-ng/umbrella/blob/2065c927cfe52fe72534fcc0ee74260c760d9966/packages/hiccup/src/serialize.ts#L118-L119

However, trying to use this form from rdom results in an exception in the browser:

I can't tell exactly what path the __COMMENT__ tag would follow through rdom's $el a priori, but it seems like a properly oriented document.createComment() in that function might work. If you think this approach is worth pursuing, I'd be more than glad to submit a PR.

[postspectacular]

Hi @usernolan 👋 - that's a good question and TBH the first time I ever thought about that. Taking a look at the $el it seems the problem will be that the created element (or now comment) will have to be returned, however the JS native Comment type unfortunately is not a normal Element:

https://github.com/microsoft/TypeScript/blob/cee6366c4871e42ea05a38d5011128fe932b6591/lib/lib.dom.d.ts#L3652-L3654

https://github.com/microsoft/TypeScript/blob/cee6366c4871e42ea05a38d5011128fe932b6591/lib/lib.dom.d.ts#L4858-L4967

So in short, putting in the extra check & branch to create a comment node is the easy part, but then we would also have to change the return type of $el and that's where things start to get complicated. At first glance I think it would even cause a breaking change for user component code, but first I will have to take a closer look at all the call sites and some of the secondary effects in user code in more detail... 👍

If you want to work on a PR, then by all means - happy to guide you! :)

[usernolan]

Ah, indeed. How naive of me to assume that createComment would be able to slip in there unannounced 😂. Thanks for highlighting that; it definitely makes the change more involved.

I'd love to work through it if you think that's a feature you'd want to support (consider this 1 vote in favor!). It'd be great to learn the rdom internals a bit better in any case. And any guidance would be great—I certainly wouldn't want to go in a direction you wouldn't. Naively, I'd probably start with something like type XXX = Element | Comment and see where that led. Do you think you'd follow that down or start somewhere else?

Edit: On second thought, would winding back up to Node be better? Edit: Oh, wait, maybe just a different branch in $treeTag?

[postspectacular]

Oh, damn! I didn't see your new reply and whilst checking & experimenting with a possible solution this morning, I got carried away updating/cleaning & documenting various other rdom pieces and think this all could & does work. If you want to take a look at the updated rdom-delayed-update example, you should see something like this:

Relevant source here (using newly added comment() function in thi.ng/hiccup-html:

https://github.com/thi-ng/umbrella/blob/565990fcbce9e3ef8860fdaa127ebcba22a9ef2a/examples/rdom-delayed-update/src/index.ts#L81-L82

Ps. Similar to your edited proposal, I've added a new check/branch in $tree() as well as a new $comment() function (also in the Component class)...

Would be grateful if you could do some further testing. I still think there could be some potential issues if used within $list() or $klist() components...

[postspectacular]

@usernolan - so sorry again, really didn't mean to beat you to your first PR to this project!!! 😩 I'm sure there'll another chance for you!

[usernolan]

No, this is WONDERFUL! So glad to see this all in action. Once I was able to escape the mindset of adding a branch in $el, the $comment solution became quite visible.

I’ll be able to test this in a number of scenarios over the next few days. If I find any surprises I’ll either reopen or log a new issue, vibe-dependent.

Thanks so much again for knocking this out. Next time I’ll have to be quicker on the draw. 😂🙏

[postspectacular]

Awesome & thank you (for this feature & testing)!

[usernolan]

@postspectacular I ran into an edge case that I wanted to flag in case you ever revisit this feature. It seems related to your $(k)list admonition. Minimally:

const rdomRoot = $compile([
  "div", {},
  [COMMENT, "..."],
  $replace(r.map(someComponent)),
  $switch(r, kfn1, kmap1, errComponent1),
  $switch(r, kfn2, kmap2, errComponent2)
])

What I experience here is that, on first render, the last two $switch cases are rendered in reverse order—i.e. the component from kmap2 comes before the one from kmap1 in the DOM. Reloading the page occasionally renders the components in the correct order, but the erratic behavior is consistent in the first render (across restarts, rebuilds, etc.). When the comment is omitted, I never seem to experience this behavior (over probably thousands of renders!).

I can't pretend to know what might be going on, but I thought I'd call it out. Also, I haven't seen anything similar at any point when the comment is embedded in a plain hiccup component.

Thanks again for this! I love this feature. I love this library!

[postspectacular]

Hi @usernolan - Happy New Year & apols for the slow response... Do you think you could prep a full, minimal standalone example for me to try to debug this (maybe in a gist or codesandbox somewhere)? As predicted, both of these list component wrappers just aren't prepared for any inserted comment nodes (or any other external manipulations), but maybe we can work around that... thank you (also for the kind words!) :)