feat: template-for based streaming - #474
Conversation
🦋 Changeset detectedLatest commit: 6a02913 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
556ddc6 to
ee6ebaf
Compare
| // p = e.previousSibling; | ||
| // (function initPreactPatch(d) { | ||
| // if ("htmlFor" in HTMLTemplateElement.prototype) return; | ||
| // new MutationObserver(function (records) { |
There was a problem hiding this comment.
One issue here is that this observer will be attached as soon as a <template for is present but during streaming the DOM can interleave creation with a micro-task. If that happens the current logic would make it so that we append an empty template content and due to us removing it it would never be completed. In the polyfill it's also deferred until it's ready https://github.com/GoogleChromeLabs/template-for-polyfill/blob/main/src/template-for-polyfill.ts#L22-L26
There was a problem hiding this comment.
Thank you for the review. I will work on removing issues later today. I am also seeing edge-case issues with SVG and MathML. That needs to be figured out.
There was a problem hiding this comment.
I have updated this to cover this race condition
Here is the updated size when added extra checks
Updated comparison with current client.js vs previous preact-island.
| Previous (island) | Current (DPU) | Δ | |
|---|---|---|---|
| Markers | 23 | 46 | +23 |
| Fallback | 10 | 10 | 0 |
| Subtree wrap | 54 | 29 | −25 |
| Per-boundary | 87 | 85 | −2 |
| Init script | 606 | 584 | −22 |
Hidden <div> |
18 | 0 | −18 |
| Once total | 624 | 584 | −40 |
Total wire bytes
| Boundaries | Prev | Current | Δ |
|---|---|---|---|
| 1 | 711 | 669 | −42 |
| 2 | 798 | 754 | −44 |
| 5 | 1059 | 1009 | −50 |
| 10 | 1494 | 1434 | −60 |
There was a problem hiding this comment.
we're missing some yaml thing according to CI, just wanted to flag it
There was a problem hiding this comment.
I did not run npm i on this branch causing this issue
JoviDeCroock
left a comment
JoviDeCroock
left a comment
There was a problem hiding this comment.
Looking good so far, we do need to have a harness to test this (preferrably with v11) so we know that there are no hydration missmatches when it's supported and unsupported, that we support Fragments enclosing > 1 DOM element, ....
Also will need a changeset indicating a major version
282c63f to
5be1480
Compare
| // // re-parse SVG and MathML elements so they will be rendered correctly | ||
| // for ( node of d[qsa]("svg *,math *")) { | ||
| // if (node.tagName < "a" && (node = node.closest("svg,math"))) { | ||
| // node.innerHTML += ""; |
There was a problem hiding this comment.
These lines are for handling the re-parsing of replacements inside SVGs and MathML because the template for replacement treats all elements as HTML, not SVG or MathML, so the replacement will not work
Here is a simple example page of this issue: https://fast-page-ten.vercel.app/streaming.html
|
|
||
| // let mo = new MutationObserver(initPreactPatch); | ||
| // mo.observe(d, { childList: 1, subtree: 1 }); | ||
| // d.addEventListener("DOMContentLoaded", initPreactPatch); |
There was a problem hiding this comment.
After the DOM is loaded, we run patch 1 one last time and disconnect the mutation observer, since once the page has finished streaming (loading), we will not need to make any further replacements. Since I have extra patching checks for SVG and MathML. This is more of a progressive enhancement than a polyfill.
|
I found an issue when the client resolves a boundary that is still to be streamed in, we duplicate dom |
This is not good. I have created a fix based on your description. If I can get an example, it would make verification easier (I have a unit test but have not seen it for myself in the browser). |
|
@MaxwellCohen I'll ask an agent to PR my tests (and a few fixes) to your fork, I can't currently make it to a pc 😅 |
90cb47d to
93dec46
Compare
…th still being streamed in
93dec46 to
6a02913
Compare
|
I have updated this branch and created #478 to better handle the streaming edge cases. |
2 participants
Update the template streaming markers to enable
<template for >streaming with a MutationObserver-based polyfill.This is a slightly optimized approach for template-for-based streaming compared to #468 (this is a very helpful pr)
The key changes are:
<!--$s:id-->…<!--/$s:id-->) for fallback detection; this saves a few bytes in the inline script and removes the need forname=idin<?end >tagUpdated comparison with current
client.jsvs previouspreact-island.Example:
id=5,fallback="loading...", content=<p>it works</p>, no nonce.<div>Total wire bytes
The summary was generated with the help of LLMS
You can test it out at:
I have created this demo app: https://pract-dpu.vercel.app/ repo: https://github.com/MaxwellCohen/pract-dpu
Here is a build of this code in CodeSandbox: https://ci.codesandbox.io/status/MaxwellCohen/preact-render-to-string/pr/2/builds/691245