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

feat: use comments instead of element as marker #260

Merged

Conversation

jacob-ebey
Copy link
Contributor

@jacob-ebey jacob-ebey commented Oct 31, 2022

Summary:

  • use comments to denote placeholders to not break semantic HTML structures
  • use a custom-element to handle moving into place
    • this remove the need for a mutation observer
    • seems like a cool use of a ce and I figured why not? should probably perf test it though
  • adds onError for the underlying renderToChunks implementation
  • adds renderToPipeableStream

feat: use custom element for hydration
feat: add onError to renderToChunks
feat: add renderToPipeableStream
@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2022

⚠️ No Changeset found

Latest commit: 3ad1d62

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

src/client.js Outdated
if (!i) return;

var d = this;
function f(el) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to get HTML comments than this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are worried about perf we can switch to TreeWalker, but NodeIterator has better browser support if I remember right:
image

Copy link
Contributor Author

@jacob-ebey jacob-ebey Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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                ===>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Comment on lines +12 to 32
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);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-->
Suggested change
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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaks semantic HTML for lists and whatnot. Same reason to not use a CE for denoting the fallback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think there is no way around using comments as we always risk breaking HTML or CSS selectors (:nth-child, etc)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this matter much if the elements are removed immediately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes because when the fallback is displayed (potentially a long while) styles / semantics will be broken.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

@marvinhagemeister marvinhagemeister merged commit 95c42d3 into preactjs:streaming-render Nov 8, 2022
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

Successfully merging this pull request may close these issues.

3 participants