Skip to content
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

Next.js examples update to fix duplicate meta tags #19074

Closed
2 tasks done
chrisweb opened this issue Jan 4, 2020 · 8 comments · Fixed by #19075
Closed
2 tasks done

Next.js examples update to fix duplicate meta tags #19074

chrisweb opened this issue Jan 4, 2020 · 8 comments · Fixed by #19075
Labels
docs Improvements or additions to the documentation

Comments

@chrisweb
Copy link
Contributor

chrisweb commented Jan 4, 2020

Improve both next.js examples by following best practices that have been recommended by the next.js lead

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Next.js examples have duplicate meta data:

double_meta

As you can see in my screenshot but the meta charset and the viewport tags are twice in the souce code

Expected Behavior 🤔

Only have each meta tag once in the head, as recommended here: vercel/next.js#6919 (comment)

Steps to Reproduce 🕹

https://codesandbox.io/s/quizzical-wildflower-xop10?fontsize=14&hidenavigation=1&theme=dark

Steps:

  1. Just use your developer tools and inspect the head element of the produced document, it contains two viewport meta tags and twice the charset tag

Context 🔦

I was creating a prototype with MUI to find out if it is suited for my project. I also want to use Next.js. That's when I bumped into the problem. I fixed it in my prototype and thought it might be helpful for others if I also update the MUI examples.

Your Environment 🌎

Tech Version
Material-UI v4.8.2
React v16.12.0
Next.js v9.1.7
@chrisweb
Copy link
Contributor Author

chrisweb commented Jan 4, 2020

I did a pull request to fix what's mentioned above, I moved the meta tags of the next.js examples from the _document file to the _app file as it is recommeded to do: vercel/next.js#6919 (comment)

I also
removed altogether, as it is added by default: vercel/next.js#237

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 4, 2020

They have a bunch of demos that put header in the _document. Won't it create a new font load request at each page load? https://github.com/zeit/next.js/search?p=3&q=viewport&unscoped_q=viewport

@chrisweb
Copy link
Contributor Author

chrisweb commented Jan 5, 2020

@oliviertassinari hmmm I don't know, good question, I will check this out

@chrisweb
Copy link
Contributor Author

chrisweb commented Jan 5, 2020

Do "fonts / stylesheets / scripts / ..." that are in the head element of the _app page get loaded on every nextjs page?

no, I verified this using my prototype, the css for the font only gets loaded once, for the font there were two requests one on index and again for the about page but the requests were different so I assume it's just different parts of the font that got loaded, but again only once per page, visiting any page more than once resulted in no requests

the behavior does not change if the font css meta tag is the head of _document vs the head of _app

also the observed behavior was the same in chrome v79 as well as firefox v71

I re-read the following documentations:
https://nextjs.org/docs/advanced-features/custom-app
https://nextjs.org/docs/advanced-features/custom-document
https://github.com/zeit/next.js#custom-app
https://github.com/zeit/next.js#custom-document

They clearly state that both _document as well as _app get rendered on the SSR, but the difference is that _document is not being used on the client side, while _app is

I however found another best practice concerning the title element: "why title should not be in _document but _app": vercel/next.js#4596

So personally what I would put in _document based on what I know so far, is:

  • a getInitialProps that should fetch data while the page is being SSR
  • code to alter the html or body element, for example to change the lang="en" of the html tag to something other than lang="en" or to manually add classes to the body element

what I would put in _app:

  • a getInitialProps that should fetch data for all the pages but only on the client side
  • and everything I want to be in head element

I did some runs using bombardier to measure if there is a difference by putting a lot of meta tags (>100) into _app vs putting them into _document, but my results were not conclusive on my windows machine, they were fluctuating all the time, sometimes _document with all the meta was faster and sometimes the opposite was true

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 5, 2020

By performance I had in mind between two client side pages. I don't think it will have any impact on SSR. But even for client side transitions, unless you have complex logic to render, say, the SEO headers, I don't think that the impact will be significant.

I would probably prefer _document for everything that doesn't need to change between two pages.

@chrisweb
Copy link
Contributor Author

chrisweb commented Jan 5, 2020

Yes you are right. As you mentioned there is probably not a big if any, speed difference for SSR ... but I just realized that by adding a lot of meta tags to the _app (page) I would potentially add several kilobytes of data to both the initial rendered html page (SSR) and have those tags again in _app.js which is being transfered to the client too. As opposed to putting most meta tags in _document, which means they are only being transfered in the initially rendered html page but not in _app.js, so less overhead.

Thx for the interesting exchange, I'm glad I learned a lot about nextjs

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 5, 2020

@chrisweb Thanks for sharing your exploration of the problem. I happen to have used it quite extensively in the past, it's solid. If you care about SSR performance, be very careful with Apollo. It was our first performance bottleneck, by far, in an older project, prior to the hook area (we had >1m pages crawled by google bot a day).

@chrisweb
Copy link
Contributor Author

@oliviertassinari thank you for your insight, yes playing around with apollo is next on my list, I'm eager to learn how it works and if find out if switching from a traditional REST / redux system to Apollo (with websockets?) will increase or decrease development time, maintabillity, performance, ...

@zannager zannager added the docs Improvements or additions to the documentation label Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants