-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby): decouple html activities #28648
feat(gatsby): decouple html activities #28648
Conversation
if (stage !== `develop-html`) { | ||
await deleteRenderer(rendererPath) | ||
} | ||
await deleteRenderer(rendererPath) |
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.
This is now done inside start-server
@@ -495,7 +495,7 @@ module.exports = { | |||
// Let renderDevHTML figure it out. | |||
page: undefined, | |||
store, | |||
htmlComponentRendererPath: `${program.directory}/public/render-page.js`, | |||
htmlComponentRendererPath: pageRenderer, |
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 pageRenderer
will be undefined here for the else
codepath on lines 147-153:
let pageRenderer: string
if (process.env.GATSBY_EXPERIMENTAL_DEV_SSR) {
// ...
pageRenderer = await buildRenderer(program, Stage.DevelopHTML)
// ...
} else {
// we don't set `pageRenderer` value in this codepath
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.
It should be in the same process.env.GATSBY_EXPERIMENTAL_DEV_SSR, right? I've tested it and didn't have an issue.
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.
Oh, true. Didn't see the this if
. Still surprised TS doesn't complain about this. But it's probably because renderDevHTML
is not fully typed.
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.
good point 😂
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.
Looks good to me, thanks! 👍
Description
Decouple html ssr bundling from rendering pages to get better metrics on where html rendering is slow.
Documentation
Related Issues