Weird issue with re-render containing html.node / html.for

coreyfarrell commented 3 years ago

I've been experiencing a strange issue using render to change content in a project which uses html.node.

<html>
  <head>
    <script type="module">
      import {render, html} from 'https://unpkg.com/uhtml?module';

      window.addEventListener('load', () => {
        const testTemplate = template => render(document.body, html`${template}`);
        const div = html.node`<div>reference</div>`;
        testTemplate(html`${div}`);
        testTemplate(html`${[div]}`);
      });
    </script>
  </head>
  <body>
  </body>
</html>

I know this is not a practical example, it's what I was able to create as a minimal example of this issue. During the second render the <div> is removed and an exception is thrown. This also happens if you use html.for to initialize div.

WebReflection commented 3 years ago

this is like having jQuery around trashing nodes randomnly ... if you render via uhtml, let uhtml render. If you have nodes by reference, and you force-pass these around multiple unique template, you are causing yourself issues.

uhtml owns the nodes it renders, if you move these manually, it'll break. Don't use node, use ref instead, if needed, and don't move manually nodes by reference, as it's not something this library has been designed for, quite the opposite.

edit to clarify, you can have nodes in a rendered container, you can't move nodes with your logic though, use cloneNode(true) instead, or use conditional rendering via uhtml

WebReflection commented 3 years ago

btw, you can move nodes arbitrarily after rendering, so that the differ doesn't care, but placing nodes in holes manually in multiple templates is not something I want to solve because it impacts the differ which is in the critical path.

others forked this project and uses node.remove() instead of the current differ dance, but that's a hack that underlines the DOM is manipulated in a quite dirty way.

You can query after rendering and append nodes within containers without issues, or you can use custom elements, which always render within themselves, still without issues, but unless I understand the use case behind this, I think it's parked as "won't fix", not because I cannot reproduce, simply because this violates the library principles.

Happy to reconsider if there is a reason to do so 👋

coreyfarrell commented 3 years ago

Thanks for the explanation. I've removed all usage of html.node from my application, I'll see if this resolves the issue for me.

uhtml owns the nodes it renders, if you move these manually, it'll break. Don't use node, use ref instead, if needed, and don't move manually nodes by reference, as it's not something this library has been designed for, quite the opposite.

I'm a bit confused by this. I was not using DOM methods to move anything, just calling render multiple times. I'd love to better understand this so I can avoid this issue if I ever need to use html.for.

WebReflection commented 3 years ago

html.node is an utility you should rarely use with uhtml ... it is there to create nodes directly on the fly, and every single time is a different node. It's in because there are cases where you literally have a one-off element that you manually want to append on the page (popup, modal, etc) but it should never be used in renders.

Maybe this is something I should explain more in the documentation, but the elephant in the room is that people keep trying to optimize what uhtml has been born to optimize already in core: don't ever use html.node or html.for unless you understand why you are using those functions, otherwise you are simply slowing down the render engine instead, and creating footguns when it comes to html.node.

uhtml is blazing fast by default, the html.for is there only for sortable lists, and pretty much anything else, so if you don't have drag'n'drop, sortable, dynamic lists, with nodes somehow referenced via WeakMap, or anything else, don't use html.for and code happily ever after with cleaner source, templates, and less logic to deal with, because uhtml deals with everything it has to deal with already 👋

WebReflection / uhtml

Weird issue with re-render containing html.node / html.for #39