-
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): SSR pages during development #27432
Conversation
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've added a few comments, I've tested this on my machine which is pretty strong (8 cores - 16 threads). I did not run with the flag on to see what impact this created on gatsby.
- Kent c dodds site took 10s longer bundling dev bundle
- gatsbyjs.com took me 20s longer
I would like us to get 0s slowdown before merging and I would like to put everything inside the process.env check.
|
||
const webpackActivity = report.activityTimer(`Building development bundle`, { | ||
id: `webpack-develop`, | ||
}) | ||
webpackActivity.start() | ||
|
||
initDevWorkerPool() |
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.
initDevWorkerPool() | |
if (process.env.GATSBY_EXPERIMENTAL_DEV_SSR) { | |
initDevWorkerPool() | |
} |
@@ -116,6 +83,8 @@ export async function startServer( | |||
{ parentSpan: webpackActivity.span } | |||
) | |||
|
|||
await buildRenderer(program, Stage.DevelopHTML) |
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.
await buildRenderer(program, Stage.DevelopHTML) | |
if (process.env.GATSBY_EXPERIMENTAL_DEV_SSR) { | |
await buildRenderer(program, Stage.DevelopHTML) | |
} |
let oldHash = `` | ||
let newHash = `` | ||
const runWebpack = ( | ||
compilerConfig, | ||
stage: Stage, | ||
directory | ||
): Bluebird<webpack.Stats> => | ||
new Bluebird((resolve, reject) => { | ||
webpack(compilerConfig).run((err, stats) => { | ||
if (err) { | ||
reject(err) | ||
} else { | ||
resolve(stats) | ||
} | ||
}) | ||
if (stage === `build-html`) { | ||
webpack(compilerConfig).run((err, stats) => { | ||
if (err) { | ||
return reject(err) | ||
} else { | ||
return resolve(stats) | ||
} | ||
}) | ||
} else if (stage === `develop-html`) { | ||
webpack(compilerConfig).watch( | ||
{ | ||
ignored: /node_modules/, | ||
}, | ||
(err, stats) => { | ||
if (err) { | ||
return reject(err) | ||
} else { | ||
newHash = stats.hash || `` | ||
|
||
// Make sure we use the latest version during development | ||
if (oldHash !== `` && newHash !== oldHash) { | ||
restartWorker(`${directory}/public/render-page.js`) | ||
} | ||
|
||
oldHash = newHash | ||
|
||
return resolve(stats) | ||
} | ||
} | ||
) | ||
} | ||
}) | ||
|
||
const doBuildRenderer = async ( | ||
{ directory }: IProgram, | ||
webpackConfig: webpack.Configuration | ||
webpackConfig: webpack.Configuration, | ||
stage: Stage | ||
): Promise<string> => { | ||
const stats = await runWebpack(webpackConfig) | ||
const stats = await runWebpack(webpackConfig, stage, directory) | ||
if (stats.hasErrors()) { | ||
reporter.panic( | ||
structureWebpackErrors(`build-html`, stats.compilation.errors) | ||
) | ||
reporter.panic(structureWebpackErrors(stage, stats.compilation.errors)) | ||
} | ||
|
||
// render-page.js is hard coded in webpack.config | ||
return `${directory}/public/render-page.js` | ||
} |
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.
Diffs don't work well but I would go this route as it makes it a little bit more verbose. You'll faster see what the difference is between stages
let oldHash = ``
let newHash = ``
const runWebpack = (
compilerConfig
): Promise<webpack.Stats> =>
new Promise((resolve, reject) => {
webpack(compilerConfig).run((err, stats) => {
if (err) {
return reject(err)
} else {
return resolve(stats)
}
})
})
const watchWebpack = (
compilerConfig,
directory
): Promise<webpack.Stats> =>
new Promise((resolve, reject) => {
webpack(compilerConfig).watch(
{
ignored: /node_modules/,
},
(err, stats) => {
if (err) {
return reject(err)
} else {
newHash = stats.hash || ``
// Make sure we use the latest version during development
if (oldHash !== `` && newHash !== oldHash) {
restartWorker(`${directory}/public/render-page.js`)
}
oldHash = newHash
return resolve(stats)
}
}
)
}
const doBuildRenderer = async (
{ directory }: IProgram,
webpackConfig: webpack.Configuration,
stage: Stage
): Promise<string> => {
let stats
if (stage === 'develop-html') {
stats = await watchWebpack(webpackConfig, directory)
} else {
stats = await runWebpack(webpackConfig)
}
if (stats.hasErrors()) {
reporter.panic(structureWebpackErrors(stage, stats.compilation.errors))
}
// render-page.js is hard coded in webpack.config
return `${directory}/public/render-page.js`
}
try { | ||
const renderResponse = await renderDevHTML({ | ||
path: genericHtmlPath, | ||
// Let renderDevHTML figure it out. | ||
page: undefined, | ||
store, | ||
htmlComponentRendererPath: `${program.directory}/public/render-page.js`, | ||
directory: program.directory, | ||
}) | ||
const status = process.env.GATSBY_EXPERIMENTAL_DEV_SSR ? 404 : 200 | ||
res.status(status).send(renderResponse) | ||
} catch (e) { | ||
report.error(e) | ||
res.send(e).status(500) | ||
} |
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.
Can we keep the old code to make sure we're not changing behavior?
import { buildHTML } from "../commands/build-html" | ||
import { withBasePath } from "../utils/path" | ||
import { buildRenderer } from "../commands/build-html" | ||
import { renderDevHTML, initDevWorkerPool } from "./dev-ssr/render-dev-html" |
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.
Can we do require() inside the if statements below so we only load them when necessary, to keep side-effects at bay
Co-authored-by: Ward Peeters <ward@coding-tech.com>
😆 I spent an hour trying to figure out why the time to bundle got longer with this PR and... it turned out it didn't — on master we just don't include the time to create the dev ssr bundle in the "Building development bundle" activity. Mystery solved! |
I'm leaving the change to the develop activity as a) it's more accurate and b) currently there's no output while the dev-ssr bundle is being created so there's an odd long-ish pause during that time. |
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.
Awesome it works! There is some flakiness with hmr but it's behind a flag so 👍
Could you post details on the HMR flakiness to the umbrella issue? #28138 Will handle it soon |
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.
Re-approve after master mege
Hi, I did try & SSR in dev is working for me good. but what I constated:
This test isClientOnlyPage do not generate the generateBodyHTML of the page. For me my index is a clientonlypage as I use alias on it.. unfortunately for index the lack of ssr give me not a good result as I generate body css class in ssr to generate my classes I use
I m not using helmet in client code (just for optimisation purpose like no helmet in client reduce the client code result ) So the question I have: |
related: #25729
waiting
stateA sample error page