fix(open-next): allow more status codes for ISR revalidation#1121
fix(open-next): allow more status codes for ISR revalidation#1121conico974 merged 2 commits intoopennextjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 07122f0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
7a60f58 to
a53de6a
Compare
conico974
left a comment
There was a problem hiding this comment.
Looks good, just a little question about the status code used
| }[]; | ||
| } | ||
|
|
||
| const ALLOWED_STATUS_CODES = [200, 301, 302, 307, 308, 404, 410] |
There was a problem hiding this comment.
Is Next using the 410 status code? Similar question for 301/302, from what I remember, Next only uses 307/308
There was a problem hiding this comment.
Using getServerSideProps, you are able to set a custom status code. For example, on our website, we return a 410 status when content is deactivated.
While it's true that Next.js internally relies only on 307 and 308 status codes, I have included 301 and 302 to ensure full support for all HTTP redirects.
There was a problem hiding this comment.
Sorry forgot to respond here, page using getServerSideProps does not use ISR revalidation , useless to add anything for them.
There was a problem hiding this comment.
You're right. I’ve updated the logic to include 200, 404, and 307 / 308 status codes. I’ve actually been maintaining a local patch for about a year to handle edge cases where 'bad' 404s or redirects weren't revalidating properly in my app.
I can remove the 410 and 301 / 302 codes if you prefer. My initial goal was to be as exhaustive as possible regarding "allowed" codes, but your points are valid.
There was a problem hiding this comment.
Don't hesitate to ping me if I don't respond.
If you could drop the 410 and 301/302 that would be great. I don't want to include things that are not Next.js behavior here.
f7b6f12 to
0763a7c
Compare
0763a7c to
07122f0
Compare
|
Thanks for the PR @ntltd |
Allow more status codes for ISR revalidation
Problem
The revalidation handler was checking
res.statusCode !== 200to determine success. This had two issues:301,302,307,308), Not Found (404), and Gone (410) were incorrectly treated as failures and pushed tofailedRecords.Solution
Replace the
!== 200check with an explicitALLOWED_STATUSESallowlist:A revalidation is now only considered successful if both conditions are met:
ALLOWED_STATUS_CODESx-nextjs-cacheheader equalsREVALIDATEDWhy these status codes?
200— standard successful page response301, 302, 307, 308— redirects are valid ISR outcomes and should not be treated as failures404— intentional Not Found pages are a valid cached state in Next.js ISR410— Gone, used for pages that have been intentionally removedAny other status code (notably 5xx errors) will now explicitly mark the record as failed and trigger the existing retry/error reporting logic.