feat: use comments instead of element as marker#260
feat: use comments instead of element as marker#260marvinhagemeister merged 5 commits intopreactjs:streaming-renderfrom
Conversation
feat: use custom element for hydration feat: add onError to renderToChunks feat: add renderToPipeableStream
|
src/client.js
Outdated
| if (!i) return; | ||
|
|
||
| var d = this; | ||
| function f(el) { |
There was a problem hiding this comment.
Is there a better way to get HTML comments than this?
There was a problem hiding this comment.
Ok, this should be more performant now: https://github.com/preactjs/preact-render-to-string/pull/260/files#diff-627fe08d8bba6a4fa76e7c5ed36ef6f16d126733e65e19236b391de0d34f1fe0R14
Decided to go with a NodeIterator. TIL
There was a problem hiding this comment.
More benchmarks if we want to switch:
Chome:
Name Operations per second Average time, ms
walker 1.0 x 10^5 0.01 ==============================>
iterator 5.0 x 10^4 0.02 ===============>
recursion 7.7 x 10^3 0.13 ==>
Safari:
Name Operations per second Average time, ms (main.ts, line 70)
walker 9.1 x 10^4 0.01 ==============================>
iterator 6.3 x 10^4 0.02 =====================>
recursion 3.6 x 10^3 0.28 =>
Walker with a filter is also significantly faster than without and that assumption holds true for iterator as well:
Name Operations per second Average time, ms
walker filter 9.1 x 10^4 0.01 ==============================>
walker no filter 1.0 x 10^4 0.10 ===>
There was a problem hiding this comment.
Here's the source for the benchmark:
import Benchmark from "micro-benchmark";
function nest(_parent: HTMLElement, content: string, n: number) {
let _el = document.createElement("div");
_el.innerText = content;
_parent.appendChild(_el);
if (--n) nest(_el, content, n);
else {
_el.appendChild(document.createComment("FIND_ME"));
}
}
nest(document.body, "👻", 2000);
function findCommentRecursive(node: ChildNode, text: string): Node | null {
if (node.nodeType === Node.COMMENT_NODE && node.nodeValue === text) {
return node;
}
let child = node.firstChild;
while (child) {
const found = findCommentRecursive(child, text);
if (found) return found;
child = child.nextSibling;
}
return null;
}
function findCommentNodeIterator(node: ChildNode, text: string): Node | null {
const iterator = document.createNodeIterator(node, NodeFilter.SHOW_COMMENT);
let current = iterator.nextNode();
while (current) {
if (current.nodeValue === text) return current;
current = iterator.nextNode();
}
return null;
}
function findCommentTreeWalker(node: ChildNode, text: string): Node | null {
const walker = document.createTreeWalker(node, NodeFilter.SHOW_COMMENT);
let current = walker.nextNode();
while (current) {
if (current.nodeValue === text) return current;
current = walker.nextNode();
}
return null;
}
let result = Benchmark.suite({
specs: [
{
name: "recursion",
fn: () => {
findCommentRecursive(document.body, "FIND_ME");
},
},
{
name: "iterator",
fn: () => {
findCommentNodeIterator(document.body, "FIND_ME");
},
},
{
name: "walker",
fn: () => {
findCommentTreeWalker(document.body, "FIND_ME");
},
},
],
});
let report = Benchmark.report(result);
console.log(report);This reduces code and *should* also be more performant than recursive JS iteration. See: https://developer.mozilla.org/en-US/docs/Web/API/NodeIterator
| var s, | ||
| e, | ||
| c = document.createNodeIterator(document, 128); | ||
| while (c.nextNode()) { | ||
| let n = c.referenceNode; | ||
| if (n.data == 'preact-island:' + i) s = n; | ||
| else if (n.data == '/preact-island:' + i) e = n; | ||
| if (s && e) break; | ||
| } | ||
| if (s && e) { | ||
| var p = e.previousSibling; | ||
| while (p != s) { | ||
| if (!p || p == s) break; | ||
|
|
||
| e.parentNode.removeChild(p); | ||
| p = e.previousSibling; | ||
| } | ||
| while (d.firstChild) e.parentNode.insertBefore(d.firstChild, e); | ||
|
|
||
| d.parentNode.removeChild(d); | ||
| } |
There was a problem hiding this comment.
Doing a deep walk here seems expensive. What about using a hidden element to denote the start of any island, followed by a comment sibling to denote its end?
<link id="preact-island-00">
<h1>hello world</h1>
this is some text
<!--preact-island-00-->| var s, | |
| e, | |
| c = document.createNodeIterator(document, 128); | |
| while (c.nextNode()) { | |
| let n = c.referenceNode; | |
| if (n.data == 'preact-island:' + i) s = n; | |
| else if (n.data == '/preact-island:' + i) e = n; | |
| if (s && e) break; | |
| } | |
| if (s && e) { | |
| var p = e.previousSibling; | |
| while (p != s) { | |
| if (!p || p == s) break; | |
| e.parentNode.removeChild(p); | |
| p = e.previousSibling; | |
| } | |
| while (d.firstChild) e.parentNode.insertBefore(d.firstChild, e); | |
| d.parentNode.removeChild(d); | |
| } | |
| var n = 'preact-island-' + id, | |
| s = document.getElementById(n), | |
| p = s.parentNode, | |
| e = s, | |
| b; | |
| while (e) { | |
| b = e.nextSibling; | |
| p.removeChild(e); | |
| if (e.nodeType === 8 && e.data === n) break; | |
| e = b; | |
| } | |
| while (b = d.firstChild) p.insertBefore(b, e); |
There was a problem hiding this comment.
Breaks semantic HTML for lists and whatnot. Same reason to not use a CE for denoting the fallback.
There was a problem hiding this comment.
Yeah I think there is no way around using comments as we always risk breaking HTML or CSS selectors (:nth-child, etc)
There was a problem hiding this comment.
Does this matter much if the elements are removed immediately?
There was a problem hiding this comment.
Yes because when the fallback is displayed (potentially a long while) styles / semantics will be broken.
marvinhagemeister
left a comment
There was a problem hiding this comment.
LGTM, the more I think about it, the more I agree that the comment approach is the right one to take. Also good idea on using CE semantics instead of a MutationObserver 👍

Summary: