Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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.

Sign up for GitHub

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

Jump to bottom

next/head should prioritize document/head over default tags #30745

Closed
TyMick opened this issue Nov 1, 2021 Discussed in #19720 · 0 comments
Closed

next/head should prioritize document/head over default tags #30745

TyMick opened this issue Nov 1, 2021 Discussed in #19720 · 0 comments
Labels
bug Issue was opened via the bug report template.

Comments

@TyMick
Copy link
Contributor

TyMick commented Nov 1, 2021

What version of Next.js are you using?

12.0.2-canary.14

What version of Node.js are you using?

14.8.1

What browser are you using?

Firefox 93.0 (64-bit)

What operating system are you using?

macOS 11.6 (20G165)

How are you deploying your application?

next dev

Describe the Bug

When I add a <meta name="viewport" content="viewport-fit=cover" /> tag to a custom _document.js (because I don't intend for that viewport value to ever change in my app), next/head still adds the default <meta name="viewport" content="width=device-width"> to my HTML and sends a warning to the console linking to an error message, which reads "Adding <meta name="viewport" ...> in pages/_document.js will lead to unexpected results since it cannot be deduped."

But that isn't true. Next.js could deduplicate these viewport meta tags, if next/head consulted a list of document/head children in its filtering process. All it rightly can't do is remove children from document/head, since _document.js is only rendered once, in the server. But nothing needs to be removed from document/head in this case—the tag that's causing the problem and needs to be removed is the default meta tag in next/head.

Expected Behavior

Next.js should pass a read-only list of document/head children to next/head's filter, and next/head should let document/head win all conflicts. And Next.js should then only warn users when they create <head> tag conflicts themselves. The process would look like this:

  1. document/head adds any elements defined within it to the initial HTML, without concerning itself about defaults or anything. No warnings necessary.
  2. The list of elements that document/head added gets passed to next/head's filter somehow.
  3. next/head filters out all elements that conflict with document/head children.
    • If a conflict filtered out was part of defaultHead, that's good.
    • If a conflict filtered out was a manually defined next/head child, then generate a warning, because the user goofed by defining conflicting tags in multiple places themselves.
  4. next/head injects the results into the page.

According to @lfades's definitions of _document.js and _app.js in #13408 (comment), _document.js is a better place for a static viewport meta tag than _app.js because it's "something that you know only needs to be added once and has to be always there". There's no reason to keep injecting it over and over with next/head if the developer knows what they want. The only situation that should generate a warning is if the developer put their own viewport meta tags in both document/head and in next/head, because then they've actually created a conflict.

This would make the purposes of document/head and next/head much clearer, turning it into

  • document/head is for things that should never ever ever change throughout your entire app.
  • next/head is for things that you might want to change sometimes.

This is in contrast to the current document/head policy, which is essentially "document/head is for things that should never ever ever change except for the things that could conflict with next/head if you do something wrong." See #9794, #13230, #13408, #26534, #27142, and #30068 for more expressions of confusion over the current document/head policy.

To Reproduce

Create a custom Document with a viewport meta tag like this:

import Document, { Html, Head, Main, NextScript } from "next/document";

export default class MyDocument extends Document {
  static async getInitialProps(ctx) {
    const initialProps = await Document.getInitialProps(ctx);
    return { ...initialProps };
  }

  render() {
    return (
      <Html>
        <Head>
          <meta name="viewport" content="viewport-fit=cover" />
        </Head>
        <body>
          <Main />
          <NextScript />
        </body>
      </Html>
    );
  }
}

Or clone https://github.com/TyMick/nextjs-document-meta-tag. Then run yarn dev, watch your terminal for the console warning, and use your browser's HTML inspector to see the conflicting viewport meta tags.
@TyMick TyMick added the bug Issue was opened via the bug report template. label Nov 1, 2021
@vercel vercel locked and limited conversation to collaborators Dec 8, 2021
@balazsorban44 balazsorban44 converted this issue into discussion #32269 Dec 8, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Assignees
No one assigned
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@TyMick