feat: set status code to 500 if unexpected error occurs before streaming in app router #56236

dpnolte · 2023-09-30T11:30:49Z

This PR therefore introduces to always set response status code to 500 unless it is a NotFoundError or RedirectError. This PR would fix issue #56235. See also: https://codesandbox.io/p/sandbox/nice-panini-2z3mcp .

Current Behavior
At the moment, when an unexpected error occurs during app server rendering, a 200 ok is returned as status code. This seems to be undesirable because of the success status CDNs will cache the error pages and crawlers will index the page considering the error content as the actual content.

Desired Behavior
This issue is related to discussion #53225. Even though I understand that the response status code cannot be set if streaming has started, in my view it would be best to set the response status to 500 whenever it can (so before the streaming has started) for SEO and (CDN) http caching. This would also be consistent with how 404s currently work; that is, response status code is set to 404 if NotFoundError occurred before streaming (related issue & PR).

Ideally, when a runtime error happens after streaming, a <meta name="robots" content="noindex" /> would also be added. But I didn't want to make the PR too complex before receiving feedback.

…before streaming

test/production/app-dir/unexpected-error/app/isr-unexpected-error/page.tsx

…ror/page.tsx Co-authored-by: Vũ Văn Dũng <me@joulev.dev>

dpnolte · 2023-10-13T18:56:20Z

@feedthejim I see that you have made some changes to this file. Even though I understand you might be very busy, could you please take a look at this PR? 🙏 It is only a small change and would help us greatly to go forward.

ijjk · 2023-10-16T08:28:35Z

Allow CI Workflow Run

approve CI run for commit: 6ed114f

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

ijjk · 2023-10-16T08:28:35Z

Allow CI Workflow Run

approve CI run for commit: 6ed114f

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

feedthejim

LGTM thanks @dpnolte

dpnolte · 2023-10-16T11:33:08Z

@feedthejim Thank you for having a look! 🙏 The pipeline failed so I synced the changes from the canary branch into my PR branch. However, It seems that another approval is needed to continue the workflow?

ijjk · 2023-10-16T11:37:38Z

Tests Passed

ijjk · 2023-10-16T11:38:11Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
buildDuration	10.4s	10.4s	N/A
buildDurationCached	6.2s	6.2s	N/A
nodeModulesSize	174 MB	174 MB	⚠️ +2.91 kB
nextStartRea..uration (ms)	515ms	518ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
199-HASH.js gzip	27.5 kB	27.5 kB	✓
3f784ff6-HASH.js gzip	53.1 kB	53.1 kB	✓
99.HASH.js gzip	182 B	182 B	✓
framework-HASH.js gzip	45.3 kB	45.3 kB	✓
main-app-HASH.js gzip	254 B	251 B	N/A
main-HASH.js gzip	32.9 kB	32.9 kB	N/A
webpack-HASH.js gzip	1.75 kB	1.75 kB	N/A
Overall change	126 kB	126 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
_app-HASH.js gzip	206 B	205 B	N/A
_error-HASH.js gzip	182 B	180 B	N/A
amp-HASH.js gzip	506 B	505 B	N/A
css-HASH.js gzip	322 B	323 B	N/A
dynamic-HASH.js gzip	2.57 kB	2.57 kB	N/A
edge-ssr-HASH.js gzip	260 B	259 B	N/A
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	369 B	369 B	✓
image-HASH.js gzip	4.35 kB	4.35 kB	N/A
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.64 kB	2.63 kB	N/A
routerDirect..HASH.js gzip	312 B	311 B	N/A
script-HASH.js gzip	385 B	384 B	N/A
withRouter-HASH.js gzip	307 B	308 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.08 kB	1.08 kB	✓

Client Build Manifests

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
_buildManifest.js gzip	485 B	482 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
index.html gzip	528 B	528 B	✓
link.html gzip	542 B	541 B	N/A
withRouter.html gzip	524 B	524 B	✓
Overall change	1.05 kB	1.05 kB	✓

Edge SSR bundle Size

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
edge-ssr.js gzip	93.7 kB	93.7 kB	N/A
page.js gzip	154 kB	154 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	dpnolte/next.js fix/500_response_status	Change
middleware-b..fest.js gzip	625 B	620 B	N/A
middleware-r..fest.js gzip	150 B	151 B	N/A
middleware.js gzip	22.5 kB	22.5 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	1.92 kB	1.92 kB	✓

Diff details

Diff for page.js

Diff too large to display

Commit: 6785496

dpnolte · 2023-10-16T16:06:33Z

@feedthejim There was an error in one of the e2e tests: https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/app/standalone.test.ts#L57-L101. The related error reported in the pipeline was: Error: An unsupported type was passed to use(): [object Object]. Before this PR changes, a 200 was returned even though the page had an error. Now a 500 is returned and it makes the test fail.

Even though I've had hard time to run this particular test locally, I've tried to fix by adding a missing async to the function that is used as input for use(): 4fe20b8
Could you please re-run the pipeline again? 🙏

dpnolte · 2023-10-17T07:16:47Z

@feedthejim the pipeline checks have passed 🎉 Would it be possible to merge this PR? 🙏

dpnolte requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners September 30, 2023 11:30

ijjk added area: tests type: next labels Sep 30, 2023

dpnolte force-pushed the fix/500_response_status branch from 993febe to bee4a54 Compare September 30, 2023 12:08

Fix(app renderer): set status code to 500 if unexpected error occurs …

a5e0553

…before streaming

dpnolte force-pushed the fix/500_response_status branch from bee4a54 to a5e0553 Compare September 30, 2023 12:51

joulev reviewed Oct 1, 2023

View reviewed changes

test/production/app-dir/unexpected-error/app/isr-unexpected-error/page.tsx Outdated Show resolved Hide resolved

dpnolte and others added 3 commits October 1, 2023 12:18

Update test/production/app-dir/unexpected-error/app/isr-unexpected-er…

e440483

…ror/page.tsx Co-authored-by: Vũ Văn Dũng <me@joulev.dev>

chore(feedback): revalidate=1 and unnecessary fragments

f59709d

Merge branch 'canary' into fix/500_response_status

b81d7e2

Merge branch 'canary' into fix/500_response_status

6ed114f

feedthejim changed the title ~~Fix(app renderer): set status code to 500 if unexpected error occurs before streaming~~ feat: set status code to 500 if unexpected error occurs before streaming in app router Oct 16, 2023

feedthejim previously approved these changes Oct 16, 2023

View reviewed changes

Merge branch 'canary' into fix/500_response_status

46082ac

feedthejim added the CI approved Approve running CI for fork label Oct 16, 2023

fix(standalone test): non promise input for use

4fe20b8

dpnolte dismissed feedthejim’s stale review via 4fe20b8 October 16, 2023 15:59

Merge branch 'canary' into fix/500_response_status

8165159

dpnolte requested a review from feedthejim October 17, 2023 07:08

feedthejim approved these changes Oct 17, 2023

View reviewed changes

Merge branch 'canary' into fix/500_response_status

6785496

feedthejim merged commit 218d070 into vercel:canary Oct 17, 2023

dpnolte deleted the fix/500_response_status branch October 18, 2023 03:50

styfle added the hacktoberfest-accepted label Oct 18, 2023

github-actions bot added the locked label Nov 3, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set status code to 500 if unexpected error occurs before streaming in app router #56236

feat: set status code to 500 if unexpected error occurs before streaming in app router #56236

dpnolte commented Sep 30, 2023 •

edited

Loading

dpnolte commented Oct 13, 2023

ijjk commented Oct 16, 2023

ijjk commented Oct 16, 2023

feedthejim left a comment

dpnolte commented Oct 16, 2023

ijjk commented Oct 16, 2023 •

edited

Loading

ijjk commented Oct 16, 2023 •

edited

Loading

dpnolte commented Oct 16, 2023

dpnolte commented Oct 17, 2023

feat: set status code to 500 if unexpected error occurs before streaming in app router #56236

feat: set status code to 500 if unexpected error occurs before streaming in app router #56236

Conversation

dpnolte commented Sep 30, 2023 • edited Loading

dpnolte commented Oct 13, 2023

ijjk commented Oct 16, 2023

ijjk commented Oct 16, 2023

feedthejim left a comment

Choose a reason for hiding this comment

dpnolte commented Oct 16, 2023

ijjk commented Oct 16, 2023 • edited Loading

Tests Passed

ijjk commented Oct 16, 2023 • edited Loading

Stats from current PR

dpnolte commented Oct 16, 2023

dpnolte commented Oct 17, 2023

dpnolte commented Sep 30, 2023 •

edited

Loading

ijjk commented Oct 16, 2023 •

edited

Loading

ijjk commented Oct 16, 2023 •

edited

Loading