Merge output: hybrid and output: static#11824
Conversation
|
c423751 to
00c0fd2
Compare
| .default(ASTRO_CONFIG_DEFAULTS.trailingSlash), | ||
| output: z | ||
| .union([z.literal('static'), z.literal('server'), z.literal('hybrid')]) | ||
| .union([z.literal('static'), z.literal('server')]) |
There was a problem hiding this comment.
This seems weird. A project with output: 'static' can have a non-static output. When I was reading the thread about this on Discord I thought that the merge would remove static and have hybrid as the default.
There was a problem hiding this comment.
The names here honestly don't matter, since they're not used for logic anymore apart from choosing the default value of prerender
My own take is that the key should be changed to something else than output, Matthew felt like it was too confusing for users if hybrid was the default etc. It's still up for debate!
There was a problem hiding this comment.
Yeah, I'd prefer to have a boolean prerenderDefault, which defaults to false. No more output.
There was a problem hiding this comment.
prerenderDefault forces the concept of prerendering onto people who just want to build a regular static (or server) site. People who build mixed sites are the exception.
I don't feel super strongly about hybrid vs static personally. I think it's ok that static has some non-static output because server can already have non-server output too.
| export function resolveInjectedRoute(entrypoint: string, root: URL, cwd?: string) { | ||
| let resolved; | ||
| try { | ||
| resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(root)] }); |
There was a problem hiding this comment.
😭 now I can no longer trick Astro into loading a virtual module as an API route.
But it's fine; this gives me an idea of how to circumvent it again.
There was a problem hiding this comment.
This code is not new, it just moved
There was a problem hiding this comment.
We have moved the adapter to https://github.com/withastro/adapters, so this PR can't be merged here anymore.
I would offer to port over the changes to the new repo for you, just let me know if you want me to do that.
| if (config.output !== 'hybrid' && config.output !== 'server') { | ||
| throw new AstroError( | ||
| 'No SSR adapter found.', | ||
| '`@astrojs/web-vitals` requires your site to be built with `hybrid` or `server` output.\n' + | ||
| 'Please add an SSR adapter: https://docs.astro.build/en/guides/server-side-rendering/', | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
This message doesn't make sense anymore, the route that web-vitals adds will cause the website to be hybrid anyway since it's not pre-rendered.
| return; | ||
| } | ||
| default: | ||
| default: // `settings.buildOutput` will always be one of the above, but TS doesn't know that |
There was a problem hiding this comment.
Isn't it a union of the two strings? TypeScript should know that, and not need the default. See https://www.typescriptlang.org/play/?#code/C4TwDgpgBAzg9gWwsAFgSwHYHMoF4oDkcGEBUAPocAO5wEDcAUIwGYCuGAxsGsVACZwAysDYsWAClSYsALliJk6bAEp5GNggBGEAE5QA3oygnY1NME4opyrCsPHTTzgEMY0IiQLzdyNrowoAEYmJ2c3Dxo6Hz8AqAAmUNMAX0ZkoA
There was a problem hiding this comment.
It's not, the default value is undefined, for the short instant where we don't know yet the output.. but we could make it default to the option of the config I think
|
I was going to resolve my blocking commit, because I saw you opened a matching PR in the adapters repo, but I see that the changes in this PR still include a vercel adapter file? |
|
There are still some things here to do, but this PR has been open for way too long and conflicts are getting harder and harder to fix. We'll merge it here and fix the few remaining points in later PRs throughout next week. |
|
During build time, on each static page I receive I used to use 'hybrid', switched to 'static' on v5 |
|
@pijusz, please open an issue with a reproduction. Leaving comments in closed issues doesn't give enough visibility to your concerns. |
It was added in withastro/astro#11824 so it seems to be since v5.
Changes
This removes
output: 'hybrid'and instead makesstaticworks like hybrid did. If no pages are server-rendered, you still get a normal .html build, like you would with the previousoutput: 'static', so in theory this is only actually breaking for users ofoutput: 'hybrid'.To implement this, a new
buildOutputproperty onAstroSettingsdetermine what kind of output should happen, and if it'sserveryou get warned appropriately that an adapter is required for the build to succeed. This property is also used in dev to warn appropriately when using features that require SSR.The scanning of if a page is pre-rendered or not is now exclusively done during the generation of the route manifest, as opposed to later during the build. This comes with numerous benefits, notably the required ability to know in advance what kind of website we're making before even starting the build, but also the ability to procure more (and accurate) information about the routes earlier anywhere the manifest is used. This could also includes integrations, in the future.
Motivation
The underlying goal of this PR is to promote the usage of SSR sparingly, putting more emphasis on Astro's ability to have only certain pages pre-rendered. We've noticed in our stats and community a great deal of users wrongfully using SSR on every page, slowing down their website unnecessarily.
By having this capacity "unlocked" by default, it also puts greater focus on Astro's ability to build all kinds of websites, not just fully-static ones, which is interesting to explore as Astro grows more and more.
Testing
Tests should pass!
Docs
Oh boy it's a big one for docs! We'll be working on this for the next few weeks and probably more, as this change end up affecting a lot of pages.