Skip to content

Stream renderer(s) can close parent elements of suspended nodes early #415

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.

Sign up for GitHub

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

Jump to bottom
Open
rschristian opened this issue Feb 7, 2025 · 2 comments
Open

Stream renderer(s) can close parent elements of suspended nodes early #415

rschristian opened this issue Feb 7, 2025 · 2 comments

Comments

@rschristian
Copy link
Member

Looking at some Remix/React-Router issues (CC @sbesh91, this isn't the only issue though) and came across this.

At the moment we don't split the document following any suspended children, only insert placeholder comments & continue on, which can cause issues when rendering a full page as react-router does.

Will need to look at other implementations yet, not familiar enough to say whether we should be splitting in all cases or if this is something we only need to deal with for body/html.

Take this input (more or less adopted from Remix/React-Router's template):

const stream = renderToReadableStream(
    <html lang="en">
        <head>
            <meta charSet="utf-8" />
        </head>
        <body>
            <div>
                <Suspense fallback="loading...">
                    <Suspender />
                </Suspense>
            </div>
        </body>
    </html>
);

This gives us stream chunks like this:

[
    "<html lang=\"en\"><head><meta charset=\"utf-8\"/></head><body><div><!--preact-island:70-->loading...<!--/preact-island:70--></div></body></html>",
    "<div hidden>",
    "<script>(function(){class e extends HTMLElement{connectedCallback(){var e=this;if(!e.isConnected)return;let t=this.getAttribute(\"data-target\");if(t){for(var r,a,i=document.createNodeIterator(document,128);i.nextNode();){let e=i.referenceNode;if(e.data==\"preact-island:\"+t?r=e:e.data==\"/preact-island:\"+t&&(a=e),r&&a)break}r&&a&&requestAnimationFrame((()=>{for(var t=a.previousSibling;t!=r&&t&&t!=r;)a.parentNode.removeChild(t),t=a.previousSibling;for(i=r;e.firstChild;)r=e.firstChild,e.removeChild(r),i.after(r),i=r;e.parentNode.removeChild(e)}))}}}customElements.define(\"preact-island\",e);}())</script>",
    "<preact-island hidden data-target=\"70\"><p>it works</p></preact-island>",
    "</div>"
]

The first chunk closes the html element (</html>) and so all the other chunks that come after will cause predicable errors when loaded in the browser:

Uncaught DOMException: CharacterData.after: Cannot have more than one Element child of a Document
@rschristian rschristian mentioned this issue Feb 7, 2025
Draft
@sbesh91
Copy link

sbesh91 commented Feb 7, 2025

You mentioned there's more Remix/react-router issues with preact-render-to-string, are there more issues posted or unposted?

@rschristian
Copy link
Member Author

rschristian commented Feb 7, 2025 •
edited
Loading

TBD yet. #417 for sure, but I'm seeing a couple other things that I'm not quite whether they're just variations of this issue or distinct entities yet.

For example, the default remix page results in a stream that ends up looking like this:

   ...
   </body>
</html>
<!--preact-island:14--><!--/preact-island:14-->
<div hidden>
...

Somehow the placeholder comment has escaped the original JSX structure altogether which is a bit surprising.

Edit: Oh, and it seems React will insert a <!DOCTYPE html> itself, thereby ensuring users aren't dropped into quirks mode which we don't do. Might not be a bad idea to prepend <html> with it though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sbesh91 @rschristian