-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
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 |
!bench server-stress |
PR BenchmarkServer stress
Main BenchmarkServer stress
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -17,9 +17,7 @@ export { | |||
Fragment, | |||
maybeRenderHead, | |||
renderTemplate as render, | |||
renderAstroTemplateResult as renderAstroComponent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
expect(chunks.length).to.equal(3); | ||
expect(chunks.length).to.equal(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Changes
Follow-up from #7730, this PR uses the new
RenderDestination
abstraction 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.write
instead.RenderInstruction
s are re-sorted and hoisted.renderComponent
API now returns a promise that eagerly renders.Testing
Existing tests should pass.
Docs
n/a.