Restore parity for Cloudflare custom worker entrypoints - #17708
Conversation
🦋 Changeset detectedLatest commit: dd89c58 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 |
github-actions
Bot
added
pkg: integration
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Merging this PR will not alter performance
Comparing 
|
# Conflicts: # packages/integrations/cloudflare/test/custom-entryfile-fetch-state.test.ts
| /** Applies the registered response finalizers in registration order. */ | ||
| finalize(response: Response): Response; | ||
| /** Returns whether the request matched an Astro route before fallback handling. */ | ||
| hasMatchedRoute(): boolean; |
There was a problem hiding this comment.
It seems we need docs. Those are public methods, we should add them alongside rewrite() in https://docs.astro.build/en/reference/modules/astro-fetch/#methods
# Conflicts: # packages/integrations/cloudflare/src/index.ts
| @@ -0,0 +1,26 @@ | |||
| --- | |||
| 'astro': minor | |||
| '@astrojs/cloudflare': patch | |||
There was a problem hiding this comment.
The minor is only for astro though. Cloudflare is just a fix that requires this new minor
There was a problem hiding this comment.
Are you saying that the Cloudflare adapter should also be a minor?
There was a problem hiding this comment.
I think so, because without the astro new minor, the "parity" can't be restored. At least, that's what I understood from the PR. Which means peerDependencies#astro of the @astrojs/cloudflare package should be 7.3 (the upcoming minor)
There was a problem hiding this comment.
Makes sense, will update.
| 'astro': minor | ||
| '@astrojs/cloudflare': patch |
| * Registers a finalizer that runs after Astro renders a response. | ||
| * Finalizers run in registration order and may mutate or replace the response. | ||
| */ | ||
| addResponseFinalizer(finalizer: (response: Response) => Response): void; |
There was a problem hiding this comment.
I don't understand why this exists. What about middleware? This is a provocative question, because docs don't mention that.
I believe it's important you highlight the differences.
There was a problem hiding this comment.
Do you mean Astro middleware?
There was a problem hiding this comment.
Just to answer here, the reason not to use Astro middleware doesn't change with this feature, it's the same reason as before, namely that:
- Astro middleware doesn't give you control over the order that Astro features run (ie you can't run your own code before Actions).
- Astro middleware is not the Fetch API and is not compatible with Hono (and its ecosystem).
This feature is intended to solve something that didn't get solved with the v1 of advanced routing, there not being a pattern for handling responses. So handlers that need a response are doing it different ways:
- i18n takes a Response: https://github.com/withastro/astro/blob/main/packages/astro/src/core/fetch/index.ts#L115-L119
- cache takes a callback that returns a Response: https://github.com/withastro/astro/blob/main/packages/astro/src/core/fetch/index.ts#L127-L129
So after this, these APIs and similar can just take take a FetchState.
There was a problem hiding this comment.
Yes I meant Astro middleware.
Thank you for the explanation. If I can give you and advice, I would try to explain why this feature exists and what it solves. Something that the middleware can't. It seems to be a very advanced API though, right?
There was a problem hiding this comment.
There's nothing that this new addResponseFinalizer API can do that middleware can't, middleware can mutate responses. But advanced routing can do things middleware can't, and that's already explained here: https://docs.astro.build/en/guides/routing/#advanced-routing
I'm not sure if this is what you're looking for, but the reason to use addResponseFinalizer and not Astro middleware is that you are already using advanced routing. You wouldn't normally use both advanced routing and Astro middleware. You might if you have an existing project which already use Astro middleware and don't want to migrate. But I don't know that addResponseFinalizer docs is the right place to explain that.
Would adding a new "Why not Astro middleware" section to the advanced routing docs satisfy this concern?
There was a problem hiding this comment.
You wouldn't normally use both advanced routing and Astro middleware.
Why not? I thought one of the reasons for using advanced routing was to change the order of the Astro's operations (redirects, middleware, actions, etc.)
Would adding a new "Why not Astro middleware" section to the advanced routing docs satisfy this concern?
Probably yes!
|
I'm moving this back to draft based on the discussion about with @ematipico, I'm thinking about how this new API conflicts with the Hono API. |
|
Closing in favor of #17887. |
3 participants
Changes
FetchStateresponse finalizers and original-route detection for composable fetch handlers.cf()is missing four thingshandle()does: cookies/sessions, the CDNno-storedefault, the assets fallback, the prerender phase #17600.Testing
FetchStateand mutable/immutable response finalization tests.Docs