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.
| status: 200 | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's fine to keep the example like this, however I think we should expose a defineFetch (or similar) utility like we do for actions (defineAction), middleware (defineMiddleware) and so on. This will improve type-safety. For examples, things might not be clear:
fetchisasyncby designfetchmust return aResponse
There was a problem hiding this comment.
I added an explanation of the shape and why it's that way here: 239213b
Added a use of satisfies Fetchable as that aligns with how other platforms do type safety.
|
|
||
| # Example | ||
|
|
||
| The minimal API for this proposal is a `src/app.ts` file with the following shape: |
There was a problem hiding this comment.
At this point I would call it src/fetch.ts. Everything orbits around a public-facing fetch APIs and concepts. The only time we mention "app", is when we mention the App class, which is and stays an internal implementation.
Also, highlight the fact that the file can be any supported extension .js, .mjs, etc.
| 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 ;)
Summary
Provide a
src/app.tsto allow full control over Astro rendering pipeline.Links