feat: middleware and virtual routes#10206
Conversation
🦋 Changeset detectedLatest commit: a86386b 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 |
9bce54c to
9f77710
Compare
|
|
||
| it('does not return custom headers for invalid URLs', async () => { | ||
| it('returns custom headers in the default 404 response', async () => { | ||
| const result = await fixture.fetch('/bad-url'); | ||
| assert.equal(result.status, 404); | ||
| assert.equal(Object.fromEntries(result.headers).hasOwnProperty('x-astro'), false); | ||
| assert.equal(Object.fromEntries(result.headers).hasOwnProperty('x-astro'), true); | ||
| }); |
There was a problem hiding this comment.
The test checks that configured headers added in config.server.headers are NOT added in the 404 responses.
This behavior was limited to astro's default 404 page - if the user added a custom 404.astro, server.headers are still added.
The new behavior is that server.headers apply to all astro-generated responses - the distinction between astro's default 404 page and user's 404.astro is gone.
|
lgtm, still in draft though and missing a changeset. |
|
Manually adding |
sarah11918
left a comment
There was a problem hiding this comment.
Just setting a block for @lilnasy ! (I'm sure docs will want to look at this anyway) 💜
OliverSpeir
left a comment
There was a problem hiding this comment.
blocking per request
.changeset/brown-pets-clean.md
Outdated
|
|
||
| The astro middleware now runs when a matching page or endpoint is not found. Previously, a `pages/404.astro` or `pages/[...catch-all].astro` route had to match to allow middleware. This is now not necessary. | ||
|
|
||
| Note that some adapters may need to update to let Astro's SSR code handle the not-found case. |
There was a problem hiding this comment.
nit: "to update.." what? It feels like we should hint at something or maybe rephrase the sentence.
It would be great if the changelog actually explains what an adapter maintainer should update in order to "fix" this.
There was a problem hiding this comment.
I feel it adequately explains the change necessary.
Adapters may look at the list of routes given by astro to create their platform-specific routing file. And the platform may then run SSR code only for routes known to be on-demand rendered. The website would then fallback to a platform-designed 404 page. SST, vercel and cloudflare (in one routing strategy) work like this.
There was a problem hiding this comment.
I feel it adequately explains the change necessary.
I am sure it is, from your point of view. However, I fear that we don't provide enough explanation to people who have less context and knowledge than you. If you think this phrase is more than enough, we should remove it.
Adapters may look at the list of routes given by astro to create their platform-specific routing file. And the platform may then run SSR code only for routes known to be on-demand rendered. The website would then fallback to a platform-designed 404 page. SST, vercel and cloudflare (in one routing strategy) work like this.
You just replied with something that we can add to the changeset 😅
|
@lilnasy What about the documentation? The template is empty. We should have a PR for that, right? |
|
Not necessarily. We don't explicitly document that middleware won't run in this case. We should also hold explicitly saying that will , until we know it works this way across adapters. |
Sorry for being pedantic, but I am a bit concerned by this kind of approach:
And how do we plan to do that? The PR doesn't say anything about it. |
|
updated the description |
ematipico
left a comment
There was a problem hiding this comment.
Thank you, Arsh, for addressing my concerns! Another minorready for the next release. Let's wait for docs :)
sarah11918
left a comment
There was a problem hiding this comment.
Tiny suggestion from docs!
|
We should update the documentation: https://docs.astro.build/en/guides/middleware/ We need to mention that now the middleware will run on 404 pages |
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
8b3378a to
a86386b
Compare
Changes
404page #8244Testing
Docs
Mentions of
404.astroor spread routes needing to exist to run middleware should be removed.