ghost commented 4 years ago

Bug report

Describe the bug

I ran into this error after updating Next.js to version 9.5.4. The problem is that you cannot use React Components inside Head in _app.js file.

Minimal Example:

const Metadata = () => (
  <>
      <base href="/" />
      <meta name="msapplication-TileColor" content="#000000" />
      {/*...*/}
  </>
)

class NextApp extends App {
  render() {
      return (
          <>
              <Head>
                  <title>Next Bug Report</title>
                  {/*Error*/}
                  <Metadata />
              </Head>
          </>
      )
  }
}

To Reproduce

Just open the dev server

Reproduction repo: next-bug-report

Screenshots

System information

Version of Next.js: [9.5.4]
Version of React/ReactDOM: [16.13.1]
Version of Node.js: [14.0.0]

danvoyce commented 4 years ago

Also having this issue when trying pass a component into the <Head>

jflayhart commented 4 years ago

Yeh this is a catch22 because we can't apply the open redirect security patch due to this major bug. Site doesn't function based on our use of scripts and other functionality that is required in the <Head> and we rely on our metadata as a SEO-driven PWA.

Any suggestions on how to work around this issue until it's fixed? Here's an example of how we use it:

const HomePage = () => (
    <>
        <Head>
            <CustomTitle /> // doesn't work
            <ThirdPartyScript /> // doesn't work
        </Head>
        <div>Homepage</div>
    </>
)

resulting in functionality issues per the runtime error. It "silently" fails in prod, meaning the page may still render depending on the Head implementation, but it completely kills dev environment per OP.

jflayhart commented 4 years ago

My understanding of the issue is this change and/or this change removed support for accepting react components as a type, whether said component is simply <title /> or an array of <meta /> and <link /> elements, it chokes as it always returns null for that component type, which is a function:

Uncaught (in promise) DOMException: Failed to execute 'createElement' on 'Document': The tag name provided ('function CustomTitle() {
  return __jsx("title", null, "HI");
}') is not a valid name.
    at reactElementToDOM (webpack-internal:///./node_modules/next/dist/client/head-manager.js:20:21)

Running forked version of nextjs locally:

I wonder if the problem is the difference between what createElement does with initialHeadEntries: HeadEntry[] vs the default head: JSX.Element[]... the latter is fine and worked but now it's serializing head in a window object 🤔

ghost commented 3 years ago

Upgrading to the last version we notice the same problem in our project, for example with

 <Head>
     <title>Page title</title>
    ... etc
     <ResourceHints />
  </Head>

ResourceHints returns an array of react element.

if there the plan is remove array/react elements support in the head that change I think should be a major version, otherwise it's a bug, btw only happens us in dev mode

alaingoga commented 3 years ago

Also what is the purpose of having all the head tags attached to the global _NEXTDATA object? In our case we have a large list of domains we prefetch, preconnect, to improve preformance and this list would increase the initial HTML of the page.

jflayhart commented 3 years ago

btw only happens us in dev mode

@vxcamiloxv at least for us, it happens in all environments (prod vs dev), it's just dev actually throws the error and kills dev environment (for us). Prod fails silently, still throwing an error in console, but it doesn't kill the site. If you look at your HTML markup, whatever was in your react component within <head> is not injected.

jflayhart commented 3 years ago

According to one of the maintainers, it's by happy accident that this ever worked. Don't think they realized they were supporting this "feature" ;)

alaingoga commented 3 years ago

@jflayhart in our case we just called a function that returns an array of links to prefetch, preconnect. so:

  <Head>
     <title>Page title</title>
    ... etc
     { getResourceHints() }
  </Head>

instead of:

  <Head>
     <title>Page title</title>
    ... etc
     <ResourceHints />
  </Head>

That fixed our problem on dev build and removed the error on console in production. Maybe that could help, in some cases.

timneutkens commented 3 years ago

According to one of the maintainers, it's by happy accident that this ever worked. Don't think they realized they were supporting this "feature" ;)

And this is documented https://nextjs.org/docs/api-reference/next/head

ghost commented 3 years ago

@timneutkens according with the link or wrapped into maximum one level of <React.Fragment> or arrays... screenshot-2020-10-15_10-50-20

so react components inside Head are supported according with that

schlosser commented 3 years ago

I'm using the next-seo (https://github.com/garmeeh/next-seo) project, and v9.5.4 breaks it entirely due to this issue. Should this plugin "never have worked" essentially? CC @garmeeh

timneutkens commented 3 years ago

so react components inside Head are supported according with that

It says this is supported:

<Head>
  <>
    <title>Hello World</title>
  </>
</Head>

And this:

<Head>
  {[
    <title>Hello World</title>
  ]}
</Head>

I'm using the next-seo (garmeeh/next-seo) project, and v9.5.4 breaks it entirely due to this issue. Should this plugin "never have worked" essentially? CC @garmeeh

I'm saying it definitely did not work as you would expect on client-side navigation if it was rendered inside of next/head.

However that is not how next-seo works afaik, it renders a list of tags, not a React component. And uses next/head directly: https://github.com/garmeeh/next-seo/blob/337ca1f9d0932caef7d768f4c67a89aa08c28b59/src/meta/nextSEO.tsx#L24-L39

timneutkens commented 3 years ago

Just created a small app to verify what I've been saying:

import Head from "next/head";

function Test() {
  return <title>Hello World</title>;
}
export default function Home() {
  return (
    <>
      <Head>
        <Test />
      </Head>
      <h1>Hello world!</h1>
    </>
  );
}

Renders <title> as part of the initial HTML response (can be observed in view-source)
Once hydrated <title> is immediately empty
If you create another page and <Link> to it you'll notice that <title> is empty when navigating client-side as well, meaning the tags were not rendered.

To further confirm this I went and added another tag as <title> has a slight special case in next/head. But the same behavior happens when you have <meta name="description" content="Description here" />, it's part of the initial HTML but nowhere else, once the browser hydrates it's removed. On client-side navigation it's not rendered.

This was tested on next@9.5.3. On 9.5.4 and later it indeed throws an error as the logic changed a bit to remove the need for next-head-count which caused issues when external scripts were injecting tags (scripts, css etc) into the <head>.

Overall we can bring the behavior that is currently there back, @jflayhart did some work for that in #17770. However that is not ideal obviously as you currently don't get <title> etc when navigating client-side / after the initial page load.

Update: just checked the array list method (which next-seo uses) too and it seems that this has the same behavior as having a React component in 9.5.3, in 9.5.4 and later it throws an error. That specific case we can definitely solve as it's just iterating over the array. I guess #17770 already covers that.

jflayhart commented 3 years ago

Yeh that makes alot of sense now, @timneutkens. Thanks for that example app.

However that is not ideal obviously as you currently don't get etc when navigating client-side / after the initial page load.</p> </blockquote> <p>Agreed, and I made changes to my PR based on your valid concerns. I hope my PR gets us to where we all want to be, especially now that I am less ignorant of the docs and more aware of the intentions behind <code>next/head</code>.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/timneutkens"><img src="https://avatars.githubusercontent.com/u/6324199?v=4" />timneutkens</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>I was discussing this with @devknoll (who opened #17920 after I talked to him). He made the very relevant point that if we fix the issue + render the head correctly on client-side navigation it'll actually further break apps that relied on the behavior we had in previous versions. E.g. what if they're injecting <code><script></code> or <code><link></code> tags that would suddenly get injected on client-side routing where they previously did not. Also <code>useContext</code> and other React hooks that would be executed wrongly. </p> <p>Even though not 100% ideal probably the best way to go would be to bring back the previous behavior and then introduce warnings at some point.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/jflayhart"><img src="https://avatars.githubusercontent.com/u/6808172?v=4" />jflayhart</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <blockquote> <p>Even though not 100% ideal probably the best way to go would be to bring back the previous behavior and then introduce warnings at some point.</p> </blockquote> <p>@timneutkens Makes sense. I tested <a href="https://github.com/vercel/next.js/pull/17920">https://github.com/vercel/next.js/pull/17920</a> and it filters out the <code>null</code> stuff correctly, so that should get us where we were pre-9.5.4 (albeit still broken)! That would be great to get that released ASAP. So maybe <a href="https://github.com/vercel/next.js/pull/17770">https://github.com/vercel/next.js/pull/17770</a> could be a long-term fix for minor/major version roadmap with a "possible breaking change" callout in the changelog?</p> <p>Although, I am confused with this statement here:</p> <blockquote> <p>further break apps that relied on the behavior we had in previous versions</p> </blockquote> <p>Are you saying that people have figured out that components in <code>next/head</code> don't render across client-side navigation, so they're creating hacky workarounds that would break with <a href="https://github.com/vercel/next.js/pull/17770">this PR</a>? It is very React to compose components like this, so I guess I fail to see how doing things right could mess things up if it's already broken. We are having the opposite issue as we thought components containing script tags <em>were</em> being injected across client-side nav, but they aren't, so we are missing important script/meta tags!</p> <p>EDIT: My point being if people are doing things the way React allows, this shouldn't be much of a breaking change, but I am likely missing something since I don't have as much insight into next/head.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/devknoll"><img src="https://avatars.githubusercontent.com/u/1477230?v=4" />devknoll</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <blockquote> <p>Are you saying that people have figured out that components in <code>next/head</code> don't render across client-side navigation, so they're creating hacky workarounds that would break with <a href="https://github.com/vercel/next.js/pull/17770">this PR</a>? I guess I fail to see how doing things right could mess things up if it's already broken. We are having the opposite issue as we thought components containing script tags <em>were</em> being injected across client-side nav, but they weren't, so we were missing important script/meta tags!</p> </blockquote> <p>The concern is that <em>both</em> are breaking changes. The <code>next-head-count</code> causes a breakage for anyone who was using custom components. Adding client side rendering could then break anyone who was accidentally relying on those to render with context or not render on the client.</p> <p>I think we should fix it but:</p> <ol> <li>We should find a way to make the client side render isomorphic</li> <li>It should be done in a major or backward compatible/opt-in way so we don’t break upgrading </li> </ol> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/devknoll"><img src="https://avatars.githubusercontent.com/u/1477230?v=4" />devknoll</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>There’s still a problem that #17920 doesn’t fix: non-title elements rendered by custom components will be orphaned, where previously they would just be deleted when Next initialized.</p> <p>I’m not sure yet how that should be addressed.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/schlosser"><img src="https://avatars.githubusercontent.com/u/2433509?v=4" />schlosser</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <blockquote> <blockquote> <p>I'm using the next-seo (garmeeh/next-seo) project, and v9.5.4 breaks it entirely due to this issue. Should this plugin "never have worked" essentially? CC @garmeeh</p> </blockquote> <p>I'm saying it definitely did not work as you would expect on client-side navigation if it was rendered inside of next/head. However that is not how next-seo works afaik, it renders a list of tags, not a React component. And uses next/head directly: <a href="https://github.com/garmeeh/next-seo/blob/337ca1f9d0932caef7d768f4c67a89aa08c28b59/src/meta/nextSEO.tsx#L24-L39">https://github.com/garmeeh/next-seo/blob/337ca1f9d0932caef7d768f4c67a89aa08c28b59/src/meta/nextSEO.tsx#L24-L39</a></p> </blockquote> <p>Closing the loop here for some that may come across this thread, I was incorrectly using <code><NextSeo></code> nested in <code><Head></code>, which threw this error.</p> <p><strong>Bad, also breaks on 9.5.4</strong></p> <pre><code class="language-jsx"><> <Head> <NextSeo {...} /> </Head> <main> <MyApp {...} /> </main> </></code></pre> <p><strong>Good, works on 9.5.4</strong></p> <pre><code class="language-jsx"><> <NextSeo {...} /> <main> <MyApp {...} /> </main> </></code></pre> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/jflayhart"><img src="https://avatars.githubusercontent.com/u/6808172?v=4" />jflayhart</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>For our e-commerce app we were lucky since we were already using stateless, functional components within <code>Head</code>. For example, we simply changed from JSX "component" syntax, <code><ThirdPartyScripts /></code>, to functional syntax <code>{ThirdPartyScripts()}</code> and all is well >=9.5.4.</p> <p>ℹ️ <a href="https://nextjs.org/docs/api-reference/next/head">As the docs say</a>, you MUST have it as direct children of Head, or wrapped at maximum in one level of <code><React.Fragment></code> or arrays. Admittedly, I was just use to React, so I threw components in Head without reading that part of the docs 😁 </p> <p>I think it's safe to say this won't be supported soon (as it officially never was), but the maintainers are aware it's desired and I bet it will be supported in some major version based on breaking changes. </p> <p><strong>For now, functions that return Fragment or array work!</strong></p> <p>Sorry to anyone who has to refactor their class components, but at the same time it's only some meta/link tags.</p> <p>P.S. At this point, @timneutkens I think we can probably call this out as a leaky bug, but hopefully support this as a react-ish feature eventually. You all will decide and make the right decision, thanks!</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/jflayhart"><img src="https://avatars.githubusercontent.com/u/6808172?v=4" />jflayhart</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>@timneutkens But how come <code>next/document</code> "Head" works, but <code>next/head</code> doesn't? We are having no problems with JSX component syntax in our custom <code>_document</code> head:</p> <pre><code>// _document.tsx import { Head } from 'next/document' <Head> <SomeScript /> <AnotherOne /> </Head></code></pre> <p>EDIT: I guess <a href="https://nextjs.org/docs/advanced-features/custom-document">the docs</a> are saying it is literally rendered as raw head html, so it works seamlessly with JSX?</p> <blockquote> <p>The <code><Head /></code> component used here is not the same one from next/head. The <code><Head /></code> component used here should only be used for any <code><head></code> code that is common for all pages. For all other cases, such as <code><title></code> tags, we recommend using next/head in your pages or components.</p> </blockquote> <p>My my... can learn a lot from re-reading documentation 😉 </p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/timneutkens"><img src="https://avatars.githubusercontent.com/u/6324199?v=4" />timneutkens</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <blockquote> <p>@timneutkens But how come <code>next/document</code> "Head" works, but <code>next/head</code> doesn't? I don't see any problems with JSX component syntax in our custom <code>_document</code> head.</p> </blockquote> <p>Because <code>pages/_document</code> is just a shell for the page and is never rendered client-side so there's no hydration involved to ensure it is kept up-to-date between page changes. We can't tell React to render into only a part of the <code><head></code>, only the full <code><head></code> which would throw away anything that was currently there. Hence why <code>next/head</code> has custom hydration bit that knows how to render React elements (which is what JSX compiles to) </p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/jflayhart"><img src="https://avatars.githubusercontent.com/u/6808172?v=4" />jflayhart</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>As another call out, for any of those who use a custom server, you could just write some express middleware to fix the security issue to properly sanitize or 404 the open redirect bug. Just saying so no one feels 100% stuck between a rock and a hard place.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/omar-dulaimi"><img src="https://avatars.githubusercontent.com/u/11743389?v=4" />omar-dulaimi</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>For me this only occurred while using Next-Seo library like this:</p> <pre><code> <Head> {withBreadCrumbs && ( <BreadcrumbJsonLd itemListElements={breadCrumbs} /> )} </Head> </code></pre> <p>But now taking it out from the Head element, it works while still remaining inside the page head.</p> <pre><code> {withBreadCrumbs && ( <BreadcrumbJsonLd itemListElements={breadCrumbs} /> )}</code></pre> <p>I guess this work since BreadcrumbJsonLd returns a <script> element and not a Head. I could be wrong.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/matfin"><img src="https://avatars.githubusercontent.com/u/1919501?v=4" />matfin</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>I had the same issue as described above with my photography website and was able to make a workaround fix thing for my use case.</p> <p>The PR I created is <a href="https://github.com/matfin/cinematt-nextjs/pull/43/files">here</a> and this contains a diff of the changes.</p> <p>A description of what I did is as follows:</p> <ul> <li> <p>I had the <code>next/head</code> import and <code><Head /></code> component in my <a href="https://github.com/matfin/cinematt-nextjs/blob/main/src/components/layout/layout.tsx">Layout</a> component, and in there, I imported my <a href="https://github.com/matfin/cinematt-nextjs/blob/main/src/components/meta/meta.tsx">Meta component</a>.</p> </li> <li> <p>Because the aforementioned <code><Meta /></code> component contained a React fragment <code><></></code> and a load of html meta tags, this caused the error because they were not directly nested under the <code><Head /></code> component.</p> </li> <li> <p>The fix was to bring the <code><Head /></code> component into the <code><Meta /></code> component and the html meta tags could be direct children.</p> </li> <li> <p>Fixing the <a href="https://github.com/matfin/cinematt-nextjs/blob/main/src/components/meta/meta.spec.tsx">Meta component tests</a> was quite tricky, because the children inside <code><Head /></code> would not render, so I had to mock the <code><Head /></code> component using <code>jest.mock('next/head')</code>.</p> </li> </ul> <p>After all that, I was able to get things working and reach full test coverage once again.</p> <p>Hope this helps someone else. An interesting challenge to solve!</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/osdevisnot"><img src="https://avatars.githubusercontent.com/u/802242?v=4" />osdevisnot</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <p>I assume this was introduced in <a href="https://github.com/vercel/next.js/pull/16758">https://github.com/vercel/next.js/pull/16758</a> and also impacts <a href="https://github.com/vercel/next.js/issues/17854">https://github.com/vercel/next.js/issues/17854</a></p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/bencodesall"><img src="https://avatars.githubusercontent.com/u/47954098?v=4" />bencodesall</a> commented <strong> 3 years ago</strong> </div> <div class="markdown-body"> <blockquote> <p>For our e-commerce app we were lucky since we were already using stateless, functional components within <code>Head</code>. For example, we simply went from JSX "component" syntax, <code><ThirdPartyScripts /></code>, to functional syntax <code>{ThirdPartyScripts()}</code> and all is well >=9.5.4.</p> <p>ℹ️ <a href="https://nextjs.org/docs/api-reference/next/head">As the docs say</a>, you MUST have it as direct children of Head, or wrapped at maximum in one level of <code><React.Fragment></code> or arrays. Admittedly, I was just use to React, so I threw components in Head without reading that part of the docs 😁</p> <p>I think it's safe to say this won't be supported soon (as it officially never was), but the maintainers are aware it's desired and I bet it will be supported in some major version based on breaking changes. <strong>For now, functions that return Fragment or array work!</strong> Sorry to anyone who has to refactor their class components, but at the same time it's only some meta/link tags.</p> <p>P.S. At this point, @timneutkens I think we can probably call this out as a leaky bug, but hopefully support this as a react-ish feature eventually. You all will decide and make the right decision, thanks!</p> </blockquote> <p>@jflayhart 100%! I was struggling with this and switching to calling as "functional" <code>{Thing()}</code> instead of component <code><Thing /></code> along with wrapping JSX return in <code><React.Fragment></code> solved all my issues.</p> </div> </div> <div class="comment"> <div class="user"> <a rel="noreferrer nofollow" target="_blank" href="https://github.com/balazsorban44"><img src="https://avatars.githubusercontent.com/u/18369201?v=4" />balazsorban44</a> commented <strong> 2 years ago</strong> </div> <div class="markdown-body"> <p>This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.</p> </div> </div> <div class="page-bar-simple"> </div> <div class="footer"> <ul class="body"> <li>© <script> document.write(new Date().getFullYear()) </script> Githubissues.</li> <li>Githubissues is a development platform for aggregating issues.</li> </ul> </div> <script src="https://cdn.jsdelivr.net/npm/jquery@3.5.1/dist/jquery.min.js"></script> <script src="/githubissues/assets/js.js"></script> <script src="/githubissues/assets/markdown.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/highlight.min.js"></script> <script src="https://cdn.jsdelivr.net/gh/highlightjs/cdn-release@11.4.0/build/languages/go.min.js"></script> <script> hljs.highlightAll(); </script> </body> </html>

vercel / next.js

Failed to execute 'createElement' on 'Document' | v9.5.4 #17721

Bug report

Describe the bug

To Reproduce

Screenshots

System information