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.

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

[examples/with-react-helmet] <title> should not be used in _document.js's <Head> #5668

Closed
tomsseisums opened this issue Nov 13, 2018 · 12 comments
Closed

[examples/with-react-helmet] <title> should not be used in _document.js's <Head> #5668

tomsseisums opened this issue Nov 13, 2018 · 12 comments

Comments

@tomsseisums
Copy link

Examples bug report

Example name

with-react-helmet

Describe the bug

Warning: <title> should not be used in _document.js's <Head>. https://err.sh/next.js/no-document-title

To Reproduce

  • hub clone zeit/next.js
  • cd next.js/examples/with-react-helmet
  • yarn install
  • yarn next
  • open http://localhost:3000

Expected behavior

I expect that the example is set up in compliance with Next's functionality.

Screenshots

N/A

System information

  • OS: macOS
  • Browser (if applies): N/A
  • Version of Next.js: 7.0.2

Additional context

I previously didn't get the Warning, but I noticed this in my production app.
Also, AFAIK, it's not only <title> related, but also <meta>'s and basically everything else that is page related rather than totally global?

I cannot seem to figure out a way how to use react-helmet in compliance with Next, though.
@lucleray lucleray mentioned this issue Nov 15, 2018
Closed
@lucleray
Copy link
Member

I created a PR to fix that, you can have a look here : #5678

@tomsseisums
Copy link
Author

tomsseisums commented Nov 15, 2018 •
edited

Ok, but isn't there a reason why default <Head> prevents the setting of <title> in _document?
I mean, with this you got rid of the warning, but doesn't it introduce other problems?

I don't know the intricacies of <Head>, that's why I'm being cautions here.

Because, even without using Helmet, the proposed solution is to set <title> in _app instead, otherwise ... <I don't know for certain>. So, it's not just about getting rid of the warning, is it?

@lucleray
Copy link
Member

The reason for the warning is explained here : #4596

react-helmet is basically an alternative of next/head. So when you use it, you shouldn't use next/head anymore and that's why <Head> should be removed from _document and replaced by Helmet's own <head> tag.

If you try to use next/head and react-helmet at the same time, they will probably conflict. Thankfully, _document.js allows you to opt-out of next/head and replace it by your preferred solution 🙂

@timneutkens
Copy link
Member

Actually this is an interesting issue, since you don't want to remove <Head> from _document.js

@Enalmada
Copy link
Contributor

Some off topic thoughts if anyone feels like chewing on them:

  • Do you think the example readme should inform new users that next already has next/head feature to avoid people that are jumping into examples they think are relevant before reading all the docs?
  • What are some things that react-helmet does that next/head doesn't? As a new user, I am curious what reasons someone would choose to use it over next/head.
  • I noticed this interesting alternative: https://www.npmjs.com/package/react-helmet-async. Would new users be better off being shown that in the next example instead?

@timneutkens
Copy link
Member

Do you think the example readme should inform new users that next already has next/head feature to avoid people that are jumping into examples they think are relevant before reading all the docs?

Definitely, adding react-helmet would increase bundle size while it's not needed because we provide next/head

What are some things that react-helmet does that next/head doesn't? As a new user, I am curious what reasons someone would choose to use it over next/head.

In pretty much all cases you don't want to use react-helmet, it's mostly for interoperating old and new code while moving from a different app to Next.js

I noticed this interesting alternative: npmjs.com/package/react-helmet-async. Would new users be better off being shown that in the next example instead?

We could, but it doesn't make much difference until we implement streaming rendering.

@tomsseisums
Copy link
Author

In pretty much all cases you don't want to use react-helmet, it's mostly for interoperating old and new code while moving from a different app to Next.js

For instance, how would I add a class to <html> from anywhere in my code?

With Helmet I can:

<Helmet>
    <html className="classname-to-be-added" />
</Helmet>

That's the sole reason I looked upon Helmet as I couldn't find a way how to do it with native Head...

@timneutkens
Copy link
Member

What's the reason for adding classes to html/body 🤔 Because of the way css inheritance works you could add classes on pages / _app.js and it'd work in the same way.

@lucleray
Copy link
Member

Maybe adding <html lang="fr"> ?

@tomsseisums
Copy link
Author

When working with modals, to add overflow: hidden, for instance, to prevent background scroll.

@codinronan
Copy link

@timneutkens
I am working on an application that needs to be interoperable with some old code, in the process of being upgraded, but that is expected to take at least another year. In the meantime, the new code is being written and deployed in pieces, and one of the things it needs to be able to do is add classes to <body>.

Right now I'm using react-helmet to accomplish this, but would love to shed that 7kb dependency :)

I might just create a simple component that goes straight to the DOM via document.body.classList on mount ... this would be nice and declarative though.

@carlosbensant carlosbensant mentioned this issue Mar 4, 2019
Closed
@Timer
Copy link
Member

Timer commented Jun 14, 2019

Solved in #7483

@Timer Timer closed this as completed Jun 14, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix react-helmet example lucleray/next.js
6 participants
@Timer @tomsseisums @Enalmada @timneutkens @lucleray @codinronan