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

Warn if user is using <title> within the _document.js #4631

Closed
wants to merge 7 commits into from
Closed

Warn if user is using <title> within the _document.js #4631

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
45a8ede
Warn if user is using <title> within the _document.js
davscro Jun 19, 2018
2e9120d
Warn if user is using <title> within the _document.js
davscro Jun 19, 2018
b94fe40
Uses React.Children.toArray because children could be and object.
davscro Jun 19, 2018
44d9b1f
Merge branch 'canary' into canary_original_fork
davscro Sep 3, 2018
4010144
Merge branch 'canary' into canary_original_fork
davscro Sep 11, 2018
74a9f27
Disable title warning in production / compile away the code
davscro Sep 12, 2018
92b6e8b
Merge branch 'canary' into canary_original_fork
davscro Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions errors/no-document-title.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Title not allowed in _document.js

#### Why This Error Occurred

Setting `<title>` in `_document.js` is a bad idea, since then it's only server rendered, but we also do client routing.

#### Possible Ways to Fix It

Move `<title>` to `_app.js`
15 changes: 10 additions & 5 deletions server/document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class Head extends Component {
if(!files || files.length === 0) {
return null
}

return files.map((file) => {
// Only render .css files here
if(!/\.css$/.exec(file)) {
Expand Down Expand Up @@ -82,7 +82,7 @@ export class Head extends Component {
if(!files || files.length === 0) {
return null
}

return files.map((file) => {
// Only render .js files here
if(!/\.js$/.exec(file)) {
Expand Down Expand Up @@ -113,7 +113,12 @@ export class Head extends Component {
{this.getPreloadMainLinks()}
{this.getCssLinks()}
{styles || null}
{this.props.children}
{process.env.NODE_ENV === 'development' && (React.Children.toArray(this.props.children)).map((child) => {
Copy link
Member

@lucleray lucleray Sep 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I missing something or this.props.children won't be rendered at all if NODE_ENV is production ? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is wrong, and uglify won't remove the block if it's not development. Instead we have to do something like this:

let children
if (process.env.NODE_ENV === 'development') {
 children = // etc etc
} else {
 children = this.props.children
}

This way uglify will remove the specific block

Copy link
Member

@lucleray lucleray Sep 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use React.Children.forEach to show the warning ? And keep {this.props.children} ?

if (process.env.NODE_ENV === 'development') {
  React.Children.forEach(this.props.children, (child) => {
    if (child.type === 'title') {
      console.warn("...")
    }
  })
}
// return ...{this.props.children}...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing 1 loop instead of 2 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But only in development 🙈

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random question - is it not better practice also to check process.env.NODE_ENV !== 'production' instead? I thought that it was safer to assume that 'production' would be present in prod. Some people might want warnings to show in 'staging' also for example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ! That's also how it was implemented 🙂

#5160 (comment)

if (child.type === 'title') {
console.warn('Warning: <title> shouldn\'t be used in _document.js. https://err.sh/next.js/no-document-title.md')
}
return child
})}
</head>
}
}
Expand Down Expand Up @@ -159,7 +164,7 @@ export class NextScript extends Component {
if(!files || files.length === 0) {
return null
}

return files.map((file) => {
// Only render .js files here
if(!/\.js$/.exec(file)) {
Expand Down Expand Up @@ -221,4 +226,4 @@ function getPagePathname (pathname) {
}

return `${pathname}.js`
}
}