Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rdom] comment nodes cause exception #367

Closed
usernolan opened this issue Nov 29, 2022 · 8 comments
Closed

[rdom] comment nodes cause exception #367

usernolan opened this issue Nov 29, 2022 · 8 comments

Comments

@usernolan
Copy link

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:

* [COMMENT, "Hello world"]
* // <!-- Hello world -->

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

Screen Shot 2022-11-29 at 11 38 24 AM

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
Copy link
Member

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
Copy link
Author

usernolan commented Nov 29, 2022

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 added a commit that referenced this issue Nov 30, 2022
- add $comment(), isComment()
- add Component.$comment() syntax sugar
- add comment check/branch in $tree()
- update args for $addChild(), $remove(), $moveTo()
- update $text(), $html() to support SVG elements
- add doc strings
@postspectacular
Copy link
Member

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:

Screen Shot 2022-11-30 at 13 08 20

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

// DOM comment (inspect in browser dev tools)
comment(`ID: ${this.user.id}`, `Name: ${this.user.name}`),

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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

@usernolan
Copy link
Author

@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
Copy link
Member

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!) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants