-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: hybrid output #6991
feat: hybrid output #6991
Conversation
🦋 Changeset detectedLatest commit: eb55a43 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 |
Extract the fallback `prerender` module meta out of the `scan` function. It shouldn't be its responsibility to handle that
const ssr = opts.settings.config.output === 'server'; | ||
const ssr = | ||
opts.settings.config.output === 'server' || | ||
(opts.settings.config.experimental.hybridOutput && opts.settings.config.output === 'hybrid'); |
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 code seems to be repeated a lot in the PR. What do you think about a src/core/prerender/
folder where we can have a bunch of utility functions that do this sort of thing?
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.
Sounds like a good idea, I'll refactor this
// TODO: remove after the experimetal phase when | ||
// it won't be needed to edit two config fields to enable the feature | ||
function isHybridMalconfigured(config: AstroConfig) { | ||
return config.experimental.hybridOutput ? config.output !== 'hybrid' : config.output === 'hybrid'; |
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 an example of another function that might be nice to live in a prerender folder.
Code looks really great overall. I left some comments more about code organization. But functionality looks perfect. Really appreciate how much work this was and how thorough the testing is. |
Also @MoustaphaDev can you add a changeset via |
Sure, I'll add that |
This PR is blocked because it contains a |
'@astrojs/cloudflare': patch | ||
'@astrojs/netlify': patch | ||
'@astrojs/vercel': patch | ||
'@astrojs/image': patch | ||
'@astrojs/deno': patch | ||
'@astrojs/node': patch |
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.
Marked these as 'patch' because I updated the warning we get when using these adapters with an unexpected output mode.
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.
Approving the docs edits!
Nice, thanks your reviews @sarah11918! |
Changes
Implementation of the hybrid rendering RFC.
This introduces the
hybrid
output format which enables prerendering by default and allows opting out of it withexport const prerender = false
.Testing
We will use the existing prerendering test fixtures, but we will make prerendering a function of a
PRERENDER
environment variable, doingexport const prerender = import.meta.env.PRERENDER
so that we can set prerendering tofalse
ortrue
depending on which feature we're testing.Docs
There's an open docs issue withastro/docs#3226