-
Notifications
You must be signed in to change notification settings - Fork 461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(prerender): skip redirects #1448
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1448 +/- ##
=======================================
Coverage 76.17% 76.17%
=======================================
Files 73 73
Lines 7437 7439 +2
Branches 728 728
=======================================
+ Hits 5665 5667 +2
Misses 1771 1771
Partials 1 1
|
if (res.status !== 200) { | ||
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Redirections | ||
const redirectCodes = [301, 302, 303, 304, 307, 308]; | ||
if (![200, ...redirectCodes].includes(res.status)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as a feature we can try to follow redirects by checking Location
header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be nice yes. I think we can merge this and do it in a separated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not too much trouble introducing in same would be nice (so final changelog wouln't be a fix+feat)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @Hebilicious are you still on this? Please let me know if needed help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't had the time to look, but I should have time to work on this in the upcoming days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge and progress...
π Linked issue
Fix #1402
Resolves #1418
β Type of change
π Description
External links that returns 200 or 30x should be treated the same way.
We might need to add an additional check for external links.
π Checklist