-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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(page): add character encoding and page titles #1380
fix(page): add character encoding and page titles #1380
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nextauthjs/next-auth/5y9nKwgdS5qCd4vK6XLGuUwrKDMH |
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.
Thanks so much! Only some small change requests. 🙂
Also note that linting has failed! Try running npm run lint --fix
before commiting.
src/server/pages/index.js
Outdated
@@ -10,7 +10,23 @@ export default function renderPage (req, res) { | |||
|
|||
res.setHeader('Content-Type', 'text/html') | |||
function send (html) { | |||
res.send(`<!DOCTYPE html><head><style type="text/css">${css()}</style><meta name="viewport" content="width=device-width, initial-scale=1"></head><body class="__next-auth-theme-${theme}"><div class="page">${html}</div></body></html>`) | |||
res.send(`<!DOCTYPE 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.
So obviously we could generate this smarter than an inline string... But for now - since it is such simple code - I think we can live with it. The reason it was on a simple line is (I guess) to make the sent html as small as possible. Could you remove the white spaces to reflect the code that was here before?
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.
Sure. I just thought it would be more readable if its multi-line.
But as you wish.
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.
It is certainly, and we should maybe create a new .html file so we get syntax highlighting and so on, but this could be a different PR. Than we should also minify that file and inject variables into it. It's just more work not indeed for this PR, but ideas are welcome! :)
src/server/pages/index.js
Outdated
<style> | ||
${css()} | ||
</style> | ||
<title>Next Auth Sign in</title> |
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.
This one is probably a good idea to add, but if you check the lines 33-36 in this file, you can see that simply setting Next Auth Sign in
is not sufficient. I think a nice way of setting a custom title for each of the page types would be if the send
function took a title
param in addition to html, and add individual titles for signin
, signout
, verifyRequest
and error
.
I would also prefer if the function signature becomes: function send({html, title})
to future proof us. #1154 would require us to send locale
or something similar here.
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.
Ok, so should I change the function's signature to function send({ html, title })
?
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. Usually I have a rule of thumb, if you need more than one param I just like to name them for convinience.
…server/pages/index.js
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.
Sorry if I explained myself poorly, my thoughts were that you also update this section:
next-auth/src/server/pages/index.js
Lines 17 to 20 in d8d497c
signin (props) { send(signin({ csrfToken, providers, callbackUrl, ...req.query, ...props })) }, | |
signout (props) { send(signout({ csrfToken, baseUrl, basePath, ...props })) }, | |
verifyRequest (props) { send(verifyRequest({ baseUrl, ...props })) }, | |
error (props) { send(error({ basePath, baseUrl, res, ...props })) } |
So it becomes something like this. (Please run the linter before commiting)
- signin (props) { send(signin({ csrfToken, providers, callbackUrl, ...req.query, ...props })) },
+ signin (props) { send({
+ html: signin({ csrfToken, providers, callbackUrl, ...req.query, ...props }),
+ title: "Sign in
+ }) },
- signout (props) { send(signout({ csrfToken, baseUrl, basePath, ...props })) },
+ signout (props) { send({
+ html: signout({ csrfToken, baseUrl, basePath, ...props }),
+ title: "Sign out
+ }) },
- verifyRequest (props) { send(verifyRequest({ baseUrl, ...props })) },
+ verifyRequest (props) { send({
+ html: verifyRequest({ baseUrl, ...props }),
+ title: "Verify request"
+ }) },
- error (props) { send(error({ basePath, baseUrl, res, ...props })) }
+ error (props) { send({
+ html: error({ basePath, baseUrl, res, ...props }),
+ title: "Error
+ }) }
No no, you explained it right, it's my dumb brain couldn't interpret it. |
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.
LGTM! Thanks 🙂
🎉 This PR is included in version 3.6.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.1.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 4.0.0-next.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Fixed the character encoding for the sign in page
Why:
Every HTML document needs to have a UTF-8 character encoding.
How:
Just added a simple
in
src/server/pages/index.js
Also added a missing title
Checklist:
Fixes #1284