Advanced Routing#1344
Conversation
There was a problem hiding this comment.
It's a good proposal as a starter! Here's the summary of my review, which we should address (hopefully before experimental release):
- It's possible we take this for granted, but the proposal doesn't mention how this new APIs function with SSR/prerendered pages. I assume it works like the middleware, but I might be wrong. I think it's worth write it down.
- I noticed some inconsistency or lack of information in the design of some APIs, for example
state.responseand context providers. I left some comments around that area. - Error handling is completely absent, and I think we should have a paragraph that explain how would that work. E.g. there's a runtime error in
fetchduring the build, what happens? - While there's some hype around hono, the RFC doesn't say why Hono and why
astro/honoinstead of@astrojs/hono.
| 1. **Adapter / platform entrypoint** (e.g. `worker.ts`) - Platform-specific logic, receives the raw platform request. | ||
| 2. **`src/app.ts`** - User's fetch handler, receives a standard `Request`. | ||
| 3. **Feature handlers** - Astro features like redirects, pages, etc. |
There was a problem hiding this comment.
We need to take edge middleware into account too.
There was a problem hiding this comment.
Added a note that platform edge middleware (Vercel, Netlify, etc.) runs before the adapter and is outside Astro's control. This proposal doesn't change how that layer works.
There was a problem hiding this comment.
That's not what I meant; my question runs deeper. I'll explain. Up until now, Astro was in charge of the middleware, completely. When edge middleware is enabled, the entire file and business logic are placed on the edge, meaning the middleware no longer runs within the Astro rendering pipeline.
However, with this feature, the user is now in charge of where the middleware runs. So if the user decides to use middleware from astro/fetch or astro/hono, and then enables Netlify edge middleware, what happens? Does middleware become a no-op? Do we have two middlewares?
It could be interesting to add a test in our adapters and see what the result is.
| app.use(actions()); | ||
| app.use(pages()); | ||
|
|
||
| export default app; |
There was a problem hiding this comment.
I would like to have a more in-depth paragraph of the functions exported from astro/hono. From the API bash, you mentioned that these functions are small wrappers around the functions exported from astro/fetch. If that's still the case, a paragraph should address the following concerns/questions:
- Since
src/app.tsruns for prerendered pages too, does that mean we spawn an Hono server while rendering them? - If
redirectsis a small wrapper ofredirectscoming fromastro/fetch, does that mean that it inherits the same restrictions? E.g. vite dependent, can't be used outside Astro and vite.
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
…js/hono rationale
|
Is https://elysiajs.com/ support planned or general 3rd party hooks to support it or are we limited to |
|
You should be able to use |
|
Can we use other languages e.g golang, rust, elixir etc to implement the request pipeline? |
| # Unresolved Questions | ||
|
|
||
| The exact shape of every feature handler is in-progress and not known. I think the shape of these can be discussed in the review process. | ||
|
|
||
| How should users call into Astro from platform-specific entrypoints? For example, a Cloudflare `worker.ts` that needs to export Durable Objects alongside Astro's handler might look something like: | ||
|
|
||
| ```ts | ||
| import { handler } from 'astro/fetch'; | ||
|
|
||
| export class MyDurableObject { ... } | ||
|
|
||
| export default { | ||
| fetch: handler | ||
| } | ||
| ``` | ||
|
|
||
| This proposal does not currently address this use case, but it may be something we want to support as part of or alongside this work. |
There was a problem hiding this comment.
I would like to bring some more light into this. Right now it looks a bit too complicated for users who use platform-specific entrypoints and advanced-routing the same time, as well as us to maintain both. If I understand everything correctly a request flow looks like this
-> raw request -> worker.ts -> handler(packages/integrations/cloudflare/src/utils/handler.ts) -> transformed request -> app.ts ->
Which would lead to the following code:
src/worker.ts
import handler from "@astrojs/cloudflare/entrypoints/server";
export default {
// raw request from the worker
async fetch(request, env, ctx) {
return handler.fetch(request, env, ctx);
},
} satisfies ExportedHandler<Env>;src/app.ts
import { FetchState, astro } from 'astro/fetch';
export default {
fetch(request: Request) {
// transformed request which comes from the handler in @astrojs/cloudflare/entrypoints/server
const state = new FetchState(request);
return astro(state);
}
}Decision Matrix (x means the file is required, if both columns have ?, one or the other can be used.):
| use-cases | needs worker.ts | needs app.ts |
|---|---|---|
| platform-specific features e.g. durable objects | x | |
| custom routing order | x | |
| platform-specific features e.g. durable objects & custom routing order | x | x |
custom fetch, e.g. /api/* |
? | ? |
| access bindings directly | ? | ? |
| use hono | ? | ? |
As you can see from the above matrix, there is not really any value for users to use advanced-routing other when they need to use a custom pipeline order. If they want to add custom handling based on a url /api/* they can use worker.ts. If they want to add or use hono, they can use worker.ts. If they want to access bindings directly they can use workers.ts. For all these use-cases they can also use app.ts, but they should prefer worker.ts since it's faster and allows for future usage of durable objects, without a new file. They only reason I can think of why they should use app.ts is if they need to change the order of astro's pipeline.
This in my opinion has two downsides, it makes this harder to document and explain for us when to use which, and we still need to keep and maintain packages/integrations/cloudflare/src/utils/handler.ts. In my opinion and in an ideally perfect world, worker.ts get's obsolete, and app.ts supports platform-specific entrypoints, including raw request handling, top-level exports e.g. Durable Objects, ideally so we can remove the packages/integrations/cloudflare/src/utils/handler.ts maintenance and keep documentation simple and lean. Users should have one file to reach for customizing everything, including platform-specific entrypoints as long as they follow the request/response design as well as astro pipeline.
There was a problem hiding this comment.
If the advanced routing entrypoint was configurable, it could point to the same file as wrangler's entrypoint. I wonder if that could work?
There was a problem hiding this comment.
Interesting idea, however reading the RFC it sounds like the incoming Request inside the fetch handler of a provider-specific entrypoint is not the same as the one app.ts expects 🤔
There was a problem hiding this comment.
I stopped calling this "application entrypoint" because that's not accurate and can't be accurate. Astro can't get in front of adapters/platforms; those will always run first since they own the platform. If you create a worker.ts then that's runs first, regardless of what Astro does, that's wrangler who makes that decision.
But to address your concerns here, I think we can document that astro/fetch does work inside of worker.ts so you should be able to use that instead of src/app.ts. I think that does make sense. In which case we probably can deprecate @astrojs/cloudflare/entrypoints/server and tell people to just use import { astro } from 'astro/fetch'; instead since there's not much code between the two anyways.
There was a problem hiding this comment.
@florian-lefebvre No that doesn't work, the Cloudflare vite plugin loads worker.ts, Astro doesn't. The layer is always going to be:
(host runtime) -> (host entrypoint) -> (adapter) -> astro/app -> src/app.ts
We have no control over the first 3 things in the chain. That's why I moved away from calling this an "entrypoint". It's an entrypoint inside of Astro's request handling (or as close as it can be).
There was a problem hiding this comment.
Is app.ts ever relevant for purely static sites? If not, that makes me wonder if this should be an adapter feature instead of core feature. Core could still provide astro/(fetch|hono) but it would only be used in adapter managed entrypoints
There was a problem hiding this comment.
Yes, redirects and i18n for example both work in static sites. Hono itself has a bunch as well, like sitemaps, and some development focused middleware.
Need to be careful to over-index on Cloudflare here, the Node adapter has no application entrypoint. Nor does the Vercel adapter.
There was a problem hiding this comment.
That is my hope, but is that true though? The handler from @astrojs/cloudflare/entrypoints/server does do some special stuff with SESSION & ASSETS binding and custom headers though, would that also work with astro/fetch? We would need to find a way to get that logic inside the functions of astro/fetch?
I'll look into this, but presumably we can replace this stuff with handlers, so if you're using Cloudflare you could do (as one example):
import { astro, FetchState } from 'astro/fetch';
import { cf } from '@astrojs/cloudflare/fetch';
export default {
fetch(request: Request) {
const state = new FetchState(request);
await cf(state);
return astro(state);
}
}There was a problem hiding this comment.
Here's a PR that adds these: withastro/astro#16729
So you could skip creating a src/app.ts and put this in your worker instead, basically equivalent:
import { astro, FetchState } from 'astro/fetch';
import { cf } from '@astrojs/cloudflare/fetch';
export default {
async fetch(request: Request, env: Env, ctx: ExecutionContext) {
const state = new FetchState(request);
const asset = await cf(state, env, ctx);
if (asset) return asset;
return astro(state);
}
}There was a problem hiding this comment.
Looks nice. That would probably replace import handler from "@astrojs/cloudflare/entrypoints/server"; in the future, which would be nice :)
|
@princemuel No, this doesn't change the fact that Astro is a JavaScript framework. |
…, handler signatures, and Hono rationale
|
|
||
| ``` | ||
| export default defineConfig({ | ||
| fetchFile: 'fetch.ts' |
There was a problem hiding this comment.
I think it should be called fetchEntrypoint to match other APIs, and allow:
- relative paths as strings
- package specifiers
- URLs
Also I don't know if fetchX is the right name but I don't have a better idea and I don't want to derail this comment ;)
There was a problem hiding this comment.
I don't think it should allow package specifiers. The primary goal of this RFC is to allow user to have control over routing. So the intent is for this to be a file in the project. It's an explicit non-goal of allowing integrations to "take over" this feature and I think allowing package specifiers would encourage that. The way for 3rd parties to participate is to export handlers/middleware that the user can user.
For this reason I want to keep it as fetchFile. It's similar to middleware, actions, and content config, all of which are name files (and not configurable at all).
|
I'm doing a call for consensus on this feature. As usually, there is a 3-day waiting period for people to raise concerns that this RFC should not be accepted, and then it will be assumed to pass. cc @ascorbic @ematipico @florian-lefebvre from TSC. |
| } | ||
| ``` | ||
|
|
||
| Note that using the `astro/hono` API the creation of FetchState is not necessary, since Hono has its own Context object; handlers can get/create a FetchState by inspecting HonoContext for a certain key (likely `astro.fetchState`). |
There was a problem hiding this comment.
Is the key an implementation detail or should it be clearly documented?
There was a problem hiding this comment.
It's public, I'll open a PR to document, meanwhile here's the RFC change: 5520551
There was a problem hiding this comment.
Ah, you know what, it's not as easy as just documenting the key. it's created lazily on first access using the internal getFetchState function. We could expose this, but I'd rather wait until someone asks for it.
There was a problem hiding this comment.
If I understand correctly, if there's no public API for it that means a package that wants to have handlers like the official ones can only implement some for use in a simple Fetchable, not in an Hono middleware. I think it should be a documented public API so both flavors have the same capabilities
There was a problem hiding this comment.
Updated the RFC:
- Implementation: Expose getFetchState() from astro/hono astro#16998
- Docs: Expose getFetchState function docs#14021
This reverts commit 5520551.
florian-lefebvre
left a comment
There was a problem hiding this comment.
I think #1344 (comment) should be addressed before I approve
* feat: unflag advanced routing ## Goal Remove the experimental.advancedRouting flag and enable the feature by default. Move fetchFile to a top-level config option. RFC: withastro/roadmap#1344 ## Decisions - fetchFile becomes a top-level config option (string | null, default 'app') instead of being nested under experimental.advancedRouting. The boolean gate is gone — the vite plugin always resolves src/app.ts if it exists. - Users who had experimental.advancedRouting: true can simply delete it. - Users who had experimental.advancedRouting: { fetchFile: 'fetch.ts' } move to the top-level: fetchFile: 'fetch.ts'. ## Changes - base.ts: Moved default and zod schema from experimental to top-level. - config.ts: Moved type from experimental to top-level with JSDoc. - vite-plugin.ts: Reads settings.config.fetchFile directly, removed advancedRoutingEnabled gate. - Example config: Removed experimental block. - reference/handlers.md: Removed experimental mention. * fix: disable fetchFile in vue app-entrypoint-css fixture The fixture has a src/app.ts that is a Vue app entrypoint, not an Astro fetch handler. With advanced routing always enabled, it was being loaded as the fetch handler, breaking the SSR test. * rename default fetch entrypoint from src/app.ts to src/fetch.ts * add changeset * Update packages/astro/src/types/public/config.ts Co-authored-by: Armand Philippot <git@armand.philippot.eu> * Update .changeset/seven-pots-begin.md Co-authored-by: Armand Philippot <git@armand.philippot.eu> * Update packages/astro/src/types/public/config.ts Co-authored-by: Armand Philippot <git@armand.philippot.eu> * Update packages/astro/src/types/public/config.ts Co-authored-by: Armand Philippot <git@armand.philippot.eu> --------- Co-authored-by: Armand Philippot <git@armand.philippot.eu>
Summary
Provide a
src/app.tsto allow full control over Astro rendering pipeline.Links