Refactor 404 and 500 approach by natemoo-re · Pull Request #7754 · withastro/astro

natemoo-re · 2023-07-21T18:17:41Z

Changes

Refactors our base app to handle 404 and 500 routes more consistently
Should now support pre-rendered 404 and 500 pages by making a new request to the origin server
Treating as a patch, but can release as minor if we're concerned about the approach?
Related to @astrojs/vercel with edge function target doesn't render 404.astro page #7735, Unable to find custom 404 page with trailing slash #7616

Testing

Tests updated!

Docs

N/A

changeset-bot · 2023-07-21T18:17:46Z

🦋 Changeset detected

Latest commit: da6d43a

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

ematipico

I like this approach a lot. Great refactor!

natemoo-re

A few updates! Moved some tests around, added a couple small fixes

packages/astro/src/core/app/index.ts

packages/integrations/node/test/fixtures/prerender-404/package.json

ematipico · 2023-08-01T14:50:02Z

packages/astro/src/core/app/index.ts

+	 * If is a known error code, try sending the according page (e.g. 404.astro / 500.astro).
+	 * This also handles pre-rendered /404 or /500 routes


Suggested change

* If is a known error code, try sending the according page (e.g. 404.astro / 500.astro).

* This also handles pre-rendered /404 or /500 routes

* If it is a known error code, try sending the according page (e.g. 404.astro / 500.astro).

* This also handles pre-rendered /404 or /500 routes

ematipico · 2023-08-01T14:53:04Z

packages/astro/src/core/app/index.ts

+		const { status, statusText, headers } = oldResponse;
+
+		return new Response(newResponse.body, {
+			status: status === 200 ? newResponse.status : status,


This is weird; why do we return newRespnose.status if we check the original staus? Can you add a comment?

ematipico · 2023-08-01T14:53:32Z

@natemoo-re I left some feedback, could you please address it?

* fix(app): refactor 404 and 500 approach * chore: refactor logic * fix: always treat error as page * test: migrate ssr-prerender-404 to node adapter * feat: merge original response metadata with error response * chore: update lockfile * chore: trigger ci * chore(lint): fix lint issue * fix: ensure merged request has proper status * fix(node): prerender test * chore: update test label * fix(node): improve 404 behavior in middleware mode * fix(vercel): improve 404 behavior * fix(netlify): improve 404 behavior * chore: update test labels * chore: force ci * chore: fix lint * fix: avoid infinite loops * test: fix failing test in Node 18 * chore: remove volta

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 21, 2023

matthewp approved these changes Jul 21, 2023

View reviewed changes

ematipico approved these changes Jul 22, 2023

View reviewed changes

natemoo-re force-pushed the fix/404-prerender branch from ddbdf29 to 896a5e8 Compare July 24, 2023 16:11

github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jul 24, 2023

natemoo-re commented Jul 24, 2023

View reviewed changes

packages/astro/src/core/app/index.ts Show resolved Hide resolved

packages/integrations/node/test/fixtures/prerender-404/package.json Show resolved Hide resolved

paulrudy mentioned this pull request Jul 24, 2023

vercel dev doesn't render 404 page when X-Content-Type-Options set to nosniff #7788

Closed

1 task

natemoo-re added 15 commits July 24, 2023 17:42

fix(app): refactor 404 and 500 approach

ab348cc

chore: refactor logic

6cd456f

fix: always treat error as page

bd7703a

test: migrate ssr-prerender-404 to node adapter

45131af

feat: merge original response metadata with error response

9a9c89f

chore: update lockfile

b60573c

chore: trigger ci

d26b4e9

chore(lint): fix lint issue

ddf29e4

fix: ensure merged request has proper status

1dd8331

fix(node): prerender test

a20974a

chore: update test label

dcf1933

fix(node): improve 404 behavior in middleware mode

6f297c7

fix(vercel): improve 404 behavior

68c6829

fix(netlify): improve 404 behavior

a210ac8

chore: update test labels

70bb342

natemoo-re force-pushed the fix/404-prerender branch from a753333 to 70bb342 Compare July 24, 2023 22:43

natemoo-re and others added 6 commits July 31, 2023 13:41

Merge branch 'main' into fix/404-prerender

0050cbd

chore: force ci

9798812

chore: fix lint

1332f08

fix: avoid infinite loops

6500aff

test: fix failing test in Node 18

63152da

Merge branch 'main' into fix/404-prerender

e46014e

chore: remove volta

da6d43a

natemoo-re merged commit 298dbb8 into main Aug 1, 2023

natemoo-re deleted the fix/404-prerender branch August 1, 2023 14:52

ematipico reviewed Aug 1, 2023

View reviewed changes

This was referenced Aug 1, 2023

[ci] release #7877

Merged

[ci] release (beta) #7897

Merged

natemoo-re added a commit that referenced this pull request Aug 1, 2023

chore: address feedback from #7754

9454c6a

natemoo-re mentioned this pull request Aug 1, 2023

Address feedback from #7754 #7906

Merged

natemoo-re added a commit that referenced this pull request Aug 1, 2023

chore: address feedback from #7754 (#7906)

70bbec9

This was referenced Aug 1, 2023

404 page missing CSS when 404 server Response redirects to static custom 404 page #7910

Closed

Cookies from middleware don't get set on custom 404 page #7880

Closed

Fix missing CSS when 404 server Response redirects to a custom 404 page #7946

Merged

lilnasy mentioned this pull request Aug 14, 2023

Prerendering corner cases #8070

Merged

2 tasks

natemoo-re mentioned this pull request Aug 14, 2023

404 page doesn't render in production mode #8054

Closed

1 task

lilnasy mentioned this pull request Aug 19, 2023

node ssr dont showing 404 page #5535

Closed

1 task

This was referenced Nov 14, 2023

Server-rendered 404 pages no longer work on Cloudflare withastro/adapters#67

Closed

fix(cloudflare): Fix 404.astro not being used when routes.strategy is set to exclude withastro/adapters#66

Merged

github-actions bot mentioned this pull request Nov 16, 2023

[ci] release withastro/adapters#70

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor 404 and 500 approach#7754

Refactor 404 and 500 approach#7754
natemoo-re merged 22 commits intomainfrom
fix/404-prerender

natemoo-re commented Jul 21, 2023 •

edited

Loading

Uh oh!

changeset-bot bot commented Jul 21, 2023 •

edited

Loading

Uh oh!

ematipico left a comment

Uh oh!

natemoo-re left a comment

Uh oh!

Uh oh!

Uh oh!

ematipico Aug 1, 2023

Uh oh!

ematipico Aug 1, 2023

Uh oh!

ematipico commented Aug 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		* If is a known error code, try sending the according page (e.g. 404.astro / 500.astro).
		* This also handles pre-rendered /404 or /500 routes

Uh oh!

Conversation

natemoo-re commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Docs

Uh oh!

changeset-bot bot commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

natemoo-re left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ematipico Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 1, 2023

Choose a reason for hiding this comment

Uh oh!

ematipico commented Aug 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

natemoo-re commented Jul 21, 2023 •

edited

Loading

changeset-bot bot commented Jul 21, 2023 •

edited

Loading