-
Notifications
You must be signed in to change notification settings - Fork 232
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
feat(sendRedirect): add refresh meta fallback for static generated responses #153
Conversation
Codecov Report
@@ Coverage Diff @@
## main nuxt/framework#153 +/- ##
==========================================
+ Coverage 65.71% 65.95% +0.24%
==========================================
Files 14 14
Lines 840 846 +6
Branches 175 175
==========================================
+ Hits 552 558 +6
Misses 118 118
Partials 170 170
Continue to review full report at Codecov.
|
src/utils/response.ts
Outdated
@@ -25,7 +25,9 @@ export function defaultContentType (event: CompatibilityEvent, type?: string) { | |||
export function sendRedirect (event: CompatibilityEvent, location: string, code = 302) { | |||
event.res.statusCode = code | |||
event.res.setHeader('Location', location) | |||
return send(event, 'Redirecting to ' + location, MIMES.html) | |||
// minimal html document that redirects on client side | |||
const html = `<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=${encodeURI(location)}"></head><body></body></html>` |
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.
Can you please also add a visual <a>
link?
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.
I've added it within a <noscript>
as otherwise I think it is a better experience not to have text flash on the screen and then off again. Let me know if you feel differently.
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.
I see but with browser fallback, there is anyway a flash to white...
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.
I thought most 'new tabs'/hard-reloads defaulted to white when loading. So there would be no flash when it loads unless we have text on it.
src/utils/response.ts
Outdated
@@ -26,7 +26,7 @@ export function sendRedirect (event: CompatibilityEvent, location: string, code | |||
event.res.statusCode = code | |||
event.res.setHeader('Location', location) | |||
// minimal html document that redirects on client side | |||
const html = `<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=${encodeURI(location)}"></head><body></body></html>` | |||
const html = `<!DOCTYPE html><html><head><meta http-equiv="refresh" content="0; url=${encodeURI(location)}"></head><body><noscript>Redirecting to <a href=${JSON.stringify(location)}>${location}</a></noscript></body></html>` |
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.
I think meta also works in case of scripts being disabled :) What i meant was to preserve current behavior of display redirect link while navigation happens (instead of blank screen)
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.
yes, I think it does too. I was thinking more of a search engine. I still think it's a worse UX to have it flash on screen (because it's meant to be an SSR redirect, and at least the white screen suggests it's 'still loading'), but will update.
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.
I believe search engines should pick it up as a normal redirect.
I have some better ideas to handle static redirects with nitro btw once we have better route rules. It can be integrated with native provider routing and extract redirects.
nuxt/nuxt#14412
Use case is (for example) statically rendered pages. Perhaps users shouldn't prerender them, but it seems a reasonable enhancement that the rendered page will also redirect to same location if saved statically and loaded in browser. White page with no text means that there should be no noticeable flash.
Might also add some text and a link within
<noscript>
?