-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Support prerendering inside of adapter runtimes #15077
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
base: next
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 6d0049b 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 |
|
| 📦 Package | 🔒 Before | 🔓 After |
|---|---|---|
| @cloudflare/kv-asset-handler | trusted-with-provenance | none |
| @cloudflare/unenv-preset | trusted-with-provenance | none |
| workerd | trusted-with-provenance | none |
| miniflare | trusted-with-provenance | none |
| youch | provenance | none |
| @cloudflare/workerd-darwin-64 | trusted-with-provenance | none |
| @cloudflare/workerd-darwin-arm64 | trusted-with-provenance | none |
| @cloudflare/workerd-linux-64 | trusted-with-provenance | none |
| @cloudflare/workerd-linux-arm64 | trusted-with-provenance | none |
| @cloudflare/workerd-windows-64 | trusted-with-provenance | none |
| wrangler | trusted-with-provenance | none |
Move environment builds to top-level builder.buildApp for framework ownership, add post plugin with enforce:'post' for manifest injection and page generation. Enables proper coordination with platform plugins.
Adds a new prerenderer API allowing adapters to control how pages are prerendered. Key changes: - Add AstroPrerenderer interface with setup/getStaticPaths/render/teardown - Add setPrerenderer to astro:build:start hook - Create default prerenderer wrapping current Node-based behavior - Refactor generate.ts to use prerenderer interface - Add astro:static-paths virtual module for runtime getStaticPaths
…erer app.match() filters out prerendered routes by default, causing 404s
- Replace getStaticPaths function with StaticPaths class - Encapsulates RouteCache internally, adapters just pass app - Track pathname→route mapping to avoid route priority issues - Use app.getPathnameFromRequest for base handling - Fall back to app.match for static routes
Redirects don't have page modules in pageMap - pipeline method handles redirects and fallbacks properly.
getStaticPaths() now returns PathWithRoute[] instead of string[]. render() receives routeData from caller, eliminating path manipulation.
The map-based route lookup added complexity and broke i18n fallback tests. Reverting to pass routeData directly to render() as before.
| import { createConsoleLogger } from './logging.js'; | ||
|
|
||
| export function createApp(dev = import.meta.env.DEV): BaseApp { | ||
| if (dev) { |
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.
DevApp (and its deps) were being bundled in the prod build. We don't minify the server builds and so tree-shaking doesn't happen. Fix was to move to a virtual module.
.changeset/custom-prerenderer-api.md
Outdated
| } | ||
| ``` | ||
|
|
||
| Also adds the `astro:static-paths` virtual module, which exports a `StaticPaths` class for adapters to collect all prerenderable paths from within their target runtime. |
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 may also need an example
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.
added one
| * @param request - The request to render | ||
| * @param routeData - The route data for this request | ||
| */ | ||
| render: (request: Request, routeData: RouteData) => Promise<Response>; |
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.
Should the shape match app.render(), ie. 2nd argument is an object with routeData and more stuff?
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.
That makes sense. I don't think we need to add any more stuff right not, but having it an object would allow expansion in the future.
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.
Updated this, thanks.
| for (const routeData of pagesToGenerate) { | ||
| if (routeData.prerender) { | ||
| // i18n domains won't work with pre rendered routes at the moment, so we need to throw an error | ||
| if (app.manifest.i18n?.domains && Object.keys(app.manifest.i18n.domains).length > 0) { |
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 may be wrong but it seems this check has been completely removed, not moved
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.
Added this back.
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
florian-lefebvre
left a comment
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 it looks pretty good! My only concern is that we may design the API to be very specific to Cloudflare because it's the only adapter we know needs this. Not for this PR but before stable, it may be useful to try migrate all our adapters to the new APIs to see if we encounter limitations
ematipico
left a comment
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.
Blocking to avoid accidental merges, since it contains pkg.pr.new packages. I understand more or less the flow of the build.
Things that are unclear or I would like to see addressed:
- More actionable errors. At the moment it's just "error occurred" without telling users what the could do (maybe using
AstroError) - New entry points. I don't know what they are, but they don't seem to be internal things, so we should look for a more future-proof solution. Should we document them?
- Tests for the new
normalizePathname
|
|
||
| // Inject manifest and content placeholders into extracted chunks | ||
| await runManifestInjection(opts, internals, extractedChunks); | ||
| settings.timer.end('SSR build'); |
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.
Is this correct? Asking because with all the refactor, it's hard to see it
| const ssrOutputs = viteBuildReturnToRollupOutputs(ssrOutput); | ||
| const ssrChunks = extractRelevantChunks(ssrOutputs, false); | ||
| ssrOutput = undefined as any; |
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.
Wouldn't it make more sense to have an if here? Plus, we assume SSR build always returns items.
Here's the change I suggest
let ssrOutput = undefined;
if (settings.buildOutput === 'static') {
ssrOutput = await builder.build(builder.environments[ASTRO_VITE_ENVIRONMENT_NAMES.ssr])
const ssrOutputs = viteBuildReturnToRollupOutputs(ssrOutput);
const ssrChunks = extractRelevantChunks(ssrOutputs, false);
ssrOutput = undefined as any;
}However, if this code was made like this for a specific reason, maybe we should add a comment
| name: MODULE_DEV_CSS_ALL, | ||
| applyToEnvironment(env) { | ||
| return ( | ||
| env.name === ASTRO_VITE_ENVIRONMENT_NAMES.ssr || |
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.
You might want to add astro too, which our second dev environment.
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.
Where do we use the astro environment? I don't think I understand what its for.
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.
AFAIK it's another runnable dev environment for when we can't use the ssr one, eg. when it's not runnable
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.
We use this environment in content collections and fonts. It's used to import modules using runner.import.
Maybe now that we know of the existence of environment.fetchModule, we might not need that anymore
| "./app/entrypoint-dev": "./dist/core/app/entrypoint-dev.js", | ||
| "./app/entrypoint-prod": "./dist/core/app/entrypoint-prod.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.
Why not entrypoint/dev and entrypoint/prod?
| "./app/entrypoint": "./dist/core/app/entrypoint.js", | ||
| "./app/entrypoint-dev": "./dist/core/app/entrypoint-dev.js", | ||
| "./app/entrypoint-prod": "./dist/core/app/entrypoint-prod.js", | ||
| "./app/manifest": "./dist/core/app/manifest.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.
Eventually we will have to document all these new entrypoints
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'll add a v6 task to review these and figure something out. A lot (if not most) of these are actually internal and are part of the export map because they need to be, but are loaded without the astro package. Maybe we need a ./internal/ folder or something to convey that.
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 have some plans around that but I think that's not blocking for v6
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.
A lot (if not most) of these are actually internal
I thought so. It's a weird situation. I don't mind keeping things internal. Should they be mentioned in the docs though?
| if (address && typeof address === 'object') { | ||
| serverUrl = `http://localhost:${address.port}`; | ||
| } else { | ||
| throw new Error('Failed to get preview server address'); |
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.
We should provide an error message that tell the user how to fix the problem, or what to do in order to fix (e.g. file a bug).
|
|
||
| async getStaticPaths(): Promise<PathWithRoute[]> { | ||
| // Call the workerd endpoint to get static paths | ||
| const response = await fetch(`${serverUrl}/__astro_static_paths`, { |
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.
Another occurrence. Yeah we really should provide more generic, future proof functions
| }); | ||
|
|
||
| if (!response.ok) { | ||
| throw new Error(`Failed to get static paths: ${response.statusText}`); |
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.
Better error message to tell the user what to do
| async teardown() { | ||
| if (previewServer) { | ||
| await previewServer.close(); | ||
| previewServer = undefined; |
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.
Maybe lease a command explaining why we explicitly set undefined
| * @param buildFormat - The build format ('file', 'directory', or 'preserve') | ||
| * @param trailingSlash - The trailing slash setting ('always', 'never', or 'ignore') | ||
| */ | ||
| export function normalizePathname( |
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.
Please add some tests
|
I think the prerendering paths should only be available during prerendering, which I don't think is currently the case. Otherwise there may be security concerns? |
Not sure it's possible. The prendering uses the preview server as foundation, which means they are always available in a way |
|
Maybe there's a way to check it's in preview with an env variable or something like that |
Summary
What Changed
AstroPrerendererinterface andsetPrerendererhook onastro:build:startto let adapters control prerendering.New API: AstroPrerenderer
Adapters can implement this interface to handle prerendering in their runtime of choice.
New Hook: setPrerenderer
Adapters can provide a prerenderer during the build.
Example: Cloudflare Prerenderer
Cloudflare now starts a workerd-backed preview server and forwards prerender requests to it.
Testing
pnpm --filter @astrojs/cloudflare testsetPrerendererAPI directly.Docs