GeezerVison is showing github.com — Issue when re-rendering empty · Issue #41 · WebReflection/uhtml
Issue when re-rendering empty #41Descriptionnote: Maybe this is not an issue, probably a wrong usage of the API, but as this wasted few hours I think it worth discussing.
I've a hooked-element/uhtml (however only uhtml seems involved in the issue) project where a component was rendering a set of items.
this.html`
<ul>
${items.map((item) =>
checkSomething(item) ? html`<li>${item.something}</li>` : html``
)}
</ul>`
);Some items must not be displayed, based on external factors.
I started this by using a react-like approach, where inside an items.map I was returning "null" to "render nothing".
I noted that returning "null" or empty strings is the wrong way, so I tried html`` and it worked.
But the issue arise when there's a re-render. In this case I get an error:
Uncaught DOMException: Node.insertBefore: Child to insert before is not a child of this node
I suspect the issue is related to multiple empty block one near another.
Here a codesandbox example:
https://codesandbox.io/s/uhtml-empty-issue-c7otb?file=/src/index.js
- first run at page load: everything is OK
- after click on "run" again: you (can) get the error
Question: although I easily fixed this by filtering out elements I don't want to display I'm wondering if this is an issue, or maybe something to be better documented. In any case I'm curious about what's wrong with html``.
Thanks! ActivityWebReflection commented on Mar 18, 2021
I easily fixed this by filtering out elements I don't want to display
It's not just that ... if you don't filter you render <li></li> for no reasons, and it doesn't matter if these are empty or not, these are DOM nodes.
However, I am not sure I understand what is item.something ... if it's a string, fallback to "", if you want the LI, otherwise filter upfront, 'cause with less nodes, you'll have better performance, despite the double loop. keul commented on Mar 18, 2021
It's not just that ... if you don't filter you render for no reasons, and it doesn't matter if these are empty or not, these are DOM nodes.
Uhm… I don't think you get what I mean. I'm "just" returning html`` , no empty LI. (Also LI is just an example, I'm not even rendering a list in my real case).
Sorry, forget the code above, probably It's just confusing. try to look at the codesandbox WebReflection commented on Mar 18, 2021
Also LI is just an example, I'm not even rendering a list in my real case
I just commented the example you provided
try to look at the codesandbox
OK, but it's the same issue ... holes are for nodes, you are still creating empty nodes for no reason, because you should filter upfront:
${numbers.filter(n => n > 0.6).map((n, index) =>
html`<li>${index} - ${n.toFixed(2)}</li>`
)}Regardless if uhtml should, or should not, behave magically (which I don't like, in general), it's conceptually wrong to just render nothing.
Filtering holed Arrays (that's what you have there, basically a [node, null, node]) means degrading performance for everyone, because when the hole is an Array, uhtml expects it to be homogeneous, otherwise it cannot diff null with anything else, as null is one reference.
In few words, I don't think I am interested in slowing uhtml due sloppy, holed, arrays, but this is surely something I could put in the README/documentation.
I hope this answers, even if it's not what you expected 👋 WebReflection commented on Mar 18, 2021 P.S. by "holes" I mean template interpolations WebReflection commented on Mar 18, 2021 P.S.2 ... if you really want to use that pattern, you can always returns instead a comment node, as that is preserved
thing ? html`<li>thing</li>` : html`<!---->` this doesn't throw 👍 (but I still strongly suggest filtering instead, it's more logical and clean) WebReflection commented on Mar 18, 2021 P.S.3 the issue, if you'd like to know details, is that:
that is emptyness ... so it cannot find any node to reproduce:
- not a text node
- nothing at all
which is why comment work, it's used as placeholder to DOM diff next render.
Since there is a trim involved, not even \n would do, or an empty space, because templates are trimmed due the way these are used most of the time:
render(document.body, html`
<div>hello</div>
`); However, as I think it's developer's duty to pass meaningful data to render, instead of expecting automatic placeholders or pass null and expect nothing, the comment hacks clearly shows that the data is wrong, not the rendering engine, as it receives data that it shouldn't, so I hope this is actually a good thing, for cleaner layouts/DOM as result.
I will check if there is an easy way to skip null while rendering though, but I won't support empty templates (or, if I do, I will convert these as null). WebReflection commented on Mar 18, 2021 P.S.4 (last one, I promise)
I had a look at udomdiff and this null idea would go on the critical path for rendering and diffing, as I need to check that b value is not null in various places ... otherwise I need to filter per each Array in search of null values ... and that's also critical path.
So, long story short, please don't pass data you are not supposed to render, and I'll explain this in the readme, after filing an issue. keul commented on Mar 18, 2021
…and I'll explain this in the readme, after filing an issue.
This was my targeting opening the issue: I was pretty sure it was a wrong usage of the lib (.filter.map just works) but it was not easy to know why it is forbidden without even getting a console warning or something.
Thanks for the long explanation the emptyness stuff, very appreciated 🙇 WebReflection commented on Mar 18, 2021 trade offs for code size and performance:
- I could put renders within a try/catch
- I could inform users where they've done something wrong
- I rather keep this project as tiny, and fast, as possible, 'cause it's one of its main strength
👋 Reacted by Marcelo Volmaro WebReflection commented on Mar 19, 2021
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|