feat: middleware and virtual routes #10206

lilnasy · 2024-02-22T22:07:22Z

Changes

Allows middleware to run for 404 responses without needing a 404.astro in the project.
Fixes Middleware isn't invoked before rendering the default 404 page #8244
Fixes Middleware doesn't redirect and serves 404 when a page doesn't exist #8226

Testing

Adapted from https://github.com/withastro/astro/pull/10185/files

Docs

Mentions of 404.astro or spread routes needing to exist to run middleware should be removed.

feat: middleware and virtual routes docs#7254

changeset-bot · 2024-02-22T22:07:26Z

🦋 Changeset detected

Latest 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

lilnasy · 2024-02-23T16:28:01Z

packages/astro/test/astro-dev-headers.test.js

-		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);
 		});


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.

matthewp · 2024-02-23T21:29:27Z

lgtm, still in draft though and missing a changeset.

lilnasy · 2024-02-23T22:24:34Z

Manually adding semver: minor because label workflow doesn't work on forks.

sarah11918

Just setting a block for @lilnasy ! (I'm sure docs will want to look at this anyway) 💜

OliverSpeir

blocking per request

ematipico · 2024-02-26T13:04:38Z

.changeset/brown-pets-clean.md

+
+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.


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.

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.

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 😅

packages/astro/src/core/routing/astro-designed-error-pages.ts

packages/astro/src/vite-plugin-astro-server/pipeline.ts

packages/astro/src/vite-plugin-astro-server/response.ts

packages/astro/src/vite-plugin-astro-server/route.ts

ematipico · 2024-02-26T13:11:42Z

@lilnasy What about the documentation? The template is empty. We should have a PR for that, right?

lilnasy · 2024-02-26T13:24:44Z

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.

ematipico · 2024-02-26T13:35:17Z

Not necessarily. We don't explicitly document that middleware won't run in this case.

Sorry for being pedantic, but I am a bit concerned by this kind of approach:

Can you update the PR template and explain why docs don't need to be updated?
Since this is a feature, we should actually document this new behaviour.

We should also hold explicitly saying that will , until we know it works this way across adapters.

And how do we plan to do that? The PR doesn't say anything about it.

lilnasy · 2024-02-26T13:38:25Z

updated the description

.changeset/brown-pets-clean.md

ematipico

Thank you, Arsh, for addressing my concerns! Another minorready for the next release. Let's wait for docs :)

sarah11918

Tiny suggestion from docs!

.changeset/brown-pets-clean.md

ematipico · 2024-03-08T10:00:26Z

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>

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 22, 2024

lilnasy force-pushed the feat/virtual-route-middleware branch 6 times, most recently from 9bce54c to 9f77710 Compare February 23, 2024 16:11

lilnasy commented Feb 23, 2024

View reviewed changes

lilnasy marked this pull request as ready for review February 23, 2024 22:17

lilnasy added the semver: minor Change triggers a `minor` release label Feb 23, 2024

sarah11918 requested changes Feb 23, 2024

View reviewed changes

OliverSpeir requested changes Feb 23, 2024

View reviewed changes

OliverSpeir approved these changes Feb 23, 2024

View reviewed changes

ematipico reviewed Feb 26, 2024

View reviewed changes

lilnasy commented Feb 26, 2024

View reviewed changes

.changeset/brown-pets-clean.md Outdated Show resolved Hide resolved

lilnasy requested a review from ematipico February 26, 2024 14:26

ematipico added this to the 4.5 milestone Feb 26, 2024

ematipico approved these changes Feb 26, 2024

View reviewed changes

sarah11918 approved these changes Mar 5, 2024

View reviewed changes

.changeset/brown-pets-clean.md Outdated Show resolved Hide resolved

ematipico modified the milestones: 4.5.0, 4.6.0 Mar 8, 2024

lilnasy and others added 10 commits March 8, 2024 16:09

add test

6a9767c

app

b15a7da

dev

b8a5715

api route -> page

38a33f9

adjust test

dbc7876

add changeset

c173983

remove any

74b2865

DEFAULT_404_COMPONENT constant

652d884

Apply suggestions from code review

5e35cb2

Apply suggestions from code review

a86386b

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

lilnasy force-pushed the feat/virtual-route-middleware branch from 8b3378a to a86386b Compare March 8, 2024 16:24

lilnasy mentioned this pull request Mar 8, 2024

feat: middleware and virtual routes withastro/docs#7254

Merged

lilnasy merged commit dc87214 into withastro:main Mar 8, 2024
13 checks passed

lilnasy deleted the feat/virtual-route-middleware branch March 8, 2024 16:39

astrobot-houston mentioned this pull request Mar 8, 2024

[ci] release #10365

Merged

sync-by-unito bot mentioned this pull request Mar 19, 2024

Bump astro from 4.0.8 to 4.5.5 GeauxWeisbeck4/homeless-hacker-blog#12

Closed

lilnasy mentioned this pull request Apr 2, 2024

feat: middleware and virtual routes #10185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: middleware and virtual routes #10206

feat: middleware and virtual routes #10206

lilnasy commented Feb 22, 2024 •

edited

Loading

changeset-bot bot commented Feb 22, 2024 •

edited

Loading

lilnasy Feb 23, 2024

matthewp commented Feb 23, 2024

lilnasy commented Feb 23, 2024

sarah11918 left a comment

OliverSpeir left a comment

ematipico Feb 26, 2024

lilnasy Feb 26, 2024

ematipico Feb 26, 2024

ematipico commented Feb 26, 2024

lilnasy commented Feb 26, 2024

ematipico commented Feb 26, 2024 •

edited

Loading

lilnasy commented Feb 26, 2024

ematipico left a comment

sarah11918 left a comment

ematipico commented Mar 8, 2024


		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.

feat: middleware and virtual routes #10206

feat: middleware and virtual routes #10206

Conversation

lilnasy commented Feb 22, 2024 • edited Loading

Changes

Testing

Docs

changeset-bot bot commented Feb 22, 2024 • edited Loading

🦋 Changeset detected

lilnasy Feb 23, 2024

Choose a reason for hiding this comment

matthewp commented Feb 23, 2024

lilnasy commented Feb 23, 2024

sarah11918 left a comment

Choose a reason for hiding this comment

OliverSpeir left a comment

Choose a reason for hiding this comment

ematipico Feb 26, 2024

Choose a reason for hiding this comment

lilnasy Feb 26, 2024

Choose a reason for hiding this comment

ematipico Feb 26, 2024

Choose a reason for hiding this comment

ematipico commented Feb 26, 2024

lilnasy commented Feb 26, 2024

ematipico commented Feb 26, 2024 • edited Loading

lilnasy commented Feb 26, 2024

ematipico left a comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

ematipico commented Mar 8, 2024

lilnasy commented Feb 22, 2024 •

edited

Loading

changeset-bot bot commented Feb 22, 2024 •

edited

Loading

ematipico commented Feb 26, 2024 •

edited

Loading