feat: advanced routing types and config gaps#16723
Conversation
🦋 Changeset detectedLatest commit: 71753fd The changes in this PR will be included in the next version bump. This PR includes changesets to release 421 packages
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 |
ematipico
left a comment
There was a problem hiding this comment.
This PR actually needs docs. The fetchFile thing isn't documented
https://docs.astro.build/en/reference/experimental-flags/advanced-routing/#adding-custom-logic
|
@ematipico docs added at withastro/docs#13890 |
| fetchFile: z | ||
| .string() | ||
| .nullable() | ||
| .optional() | ||
| .default('app'), |
There was a problem hiding this comment.
This should go under experimental.advancedRouting. We can't add new config fields like this, especially inside a patch.
There was a problem hiding this comment.
Yeah, we used to not do it that way, because would have flags like --experimental-* so it didn't make sense for experimental config to be anything more than booleans since the CLI could not convey this. BUt we stopped using CLI flags. Anyways, just an explanation for why I did it this way.
Will change to:
experimental: {
advancedRouting: {
fetchFile: ...
}
}There was a problem hiding this comment.
I know, I don't think we always added experimental CLI flags anymore
There was a problem hiding this comment.
This is changed now. Now a union of boolean or an object with this option.
| 'astro': patch | ||
| --- | ||
|
|
||
| Adds missing types and config for advanced routing: `Fetchable` export, `FetchState.response`, `App.Providers` for typed context providers, and `fetchFile` config option. Fixes Hono `cache()` middleware to follow the standard wrapper pattern. |
There was a problem hiding this comment.
nit: should this be two separate changesets? It seems the two sentences are unrelated and it might be easier to scan in the changelog if they are not in the same file.
There was a problem hiding this comment.
Yeah good point, i'll split it out into separate ones for each new thing.
|
This is ready for another review. |
ArmandPhilippot
left a comment
There was a problem hiding this comment.
Thanks, the changesets looks to me. The config.ts file as well for the content.
But, I had a doubt seeing the ### heading. I checked how this was done before and for experimental CSP, we were using a @name experimental.advancedRouting.fetchFile block, not a Markdown heading. So, I suggested changes for this.
Co-authored-by: Armand Philippot <git@armand.philippot.eu>
|
| 📦 Package | 🔒 Before | 🔓 After |
|---|---|---|
| @clack/core | trusted-with-provenance | none |
| @clack/prompts | trusted-with-provenance | none |
Co-authored-by: Armand Philippot <git@armand.philippot.eu>
Changes
Fetchabletype: Exported fromastroso users can typesrc/app.tswithsatisfies Fetchable.FetchState.response: Set byPagesHandlerandAstroMiddlewareafter rendering so callers can readstate.response.App.Providers: New extendable interface for typing custom context providers onctxandAstro.fetchFileconfig: Lets users rename the entrypoint away fromsrc/app.ts, or setnullto disable.cache()wrapper: Now follows the standard middleware pattern like the other Hono wrappers.Testing
Added 6 unit tests in
test/units/fetch/index.test.ts:state.responseis set afterpages()andmiddleware()provide/resolvelazy creation and cachingfinalizeAllruns finalize for resolved providers, skips unresolved onesDocs
Added here: withastro/docs#13890