Refactor Astro rendering to write results directly#7782
Conversation
🦋 Changeset detectedLatest commit: f0cccc8 The changes in this PR will be included in the next version bump. 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 |
github-actions
Bot
added
the
pkg: astro
|
!bench server-stress |
PR BenchmarkServer stress
Main BenchmarkServer stress
|
bluwy
left a comment
bluwy
left a comment
There was a problem hiding this comment.
Tests pass first try 😄 Below are some comments to help with review
| isHTMLString(await expression) ? expression : expression(...args); | ||
| typeof expression === 'function' ? expression(...args) : expression; |
There was a problem hiding this comment.
expression could be { render: () => {} }, so change up this handling. This code affects the Astro.slots.render API. The args are from Astro.slots.render's second parameter.
| Fragment, | ||
| maybeRenderHead, | ||
| renderTemplate as render, | ||
| renderAstroTemplateResult as renderAstroComponent, |
There was a problem hiding this comment.
astro/src/runtime/server/index.ts is publicly exported, but also slightly internal. I removed a couple of APIs as it's not needed, or the signature has changed.
| // Call component instances that might have head content to be propagated up. | ||
| const returnValue = await instance.init(result); | ||
| if (isHeadAndContent(returnValue)) { | ||
| result._metadata.extraHead.push(returnValue.head); | ||
| } |
There was a problem hiding this comment.
The code here comes from bufferHeadContent() function in render.ts. Since we render components eagerly now, we can directly invoke this here.
Could be a bit cleaner, but I think good enough for now.
| export async function renderComponent( | ||
| result: SSRResult, | ||
| displayName: string, | ||
| Component: unknown, | ||
| props: Record<string | number, any>, | ||
| slots: any = {} | ||
| ): Promise<ComponentIterable> | ComponentIterable | AstroComponentInstance { | ||
| ): Promise<RenderInstance> { |
There was a problem hiding this comment.
renderComponent is the core API for rendering any component. Now it will return a promise so it's rendered in parallel, and return a RenderInstance - a new concept added, but basically an object with { render: (destination) => void }. (destination is the RenderDestination)
| const temporaryDestination: RenderDestination = { | ||
| write(chunk) { | ||
| if (chunk instanceof Response) return; | ||
| if (typeof chunk === 'object' && 'type' in chunk && typeof chunk.type === 'string') { | ||
| if (instructions === null) { | ||
| instructions = []; | ||
| } | ||
| instructions.push(chunk); | ||
| } else { | ||
| content += chunkToString(result, chunk); | ||
| } | ||
| instructions.push(chunk); | ||
| } else { | ||
| content += chunk; | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| const renderInstance = renderSlot(result, slotted, fallback); | ||
| await renderInstance.render(temporaryDestination); |
There was a problem hiding this comment.
Slots are very tricky to render. They are split as content and instructions. content is the basic HTML, instructions are like astro island scripts/styles, and head content.
This out-of-order sorting makes it that we need a temporary destination to "collect" and sort the chunks.
| chunks.push(chunk); | ||
| } | ||
| expect(chunks.length).to.equal(3); | ||
| expect(chunks.length).to.equal(2); |
There was a problem hiding this comment.
I think because we render more stuff in sync now, it's now streamed in 2 chunks? This is the only test I had to tweak.
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
I like this new API and way of rendering things, especially the .write function!
I like it so much that when I see things like str +=, it makes me wanna cry 😢 😆
I am not a big fan of the closure approach, because it seems more a side effect. I wish we could find a better approach, to make it more "linear".
Just sharing my thoughts, overall I am happy with the implementation :)
|
Thanks for reviewing! Yeah I'm also not quite liking the closures, it's kinda the same as generator yields, but without generators. Tried to avoid that, but it's a bit hard with the slots and hoisting. Maybe we can revisit that again with this new flow. |
|
Since this is a large-ish change, I'll add a changeset to be safe so it's easier to track if it caused a regression somewhere. |
2 participants
Changes
Follow-up from #7730, this PR uses the new
RenderDestinationabstraction to render the HTML directly, instead of using generators.This is a big PR, and unfortunately hard to split off, but I'll try to leave comments for each changes.
Gist of changes:
destination.writeinstead.RenderInstructions are re-sorted and hoisted.renderComponentAPI now returns a promise that eagerly renders.Testing
Existing tests should pass.
Docs
n/a.