Warn in dev mode when script tags are added with next/head (#33968)

This commit adds a development mode warning in the console if you try to include <script> tags in next/head, e.g. ``` <Head> <script async src="..." /> </Head> ``` The warning message explains that this pattern will not work well with Suspense/streaming and recommends using the next/script component instead. TODO in follow-up PR: add same warning for stylesheets, etc ## Feature - [x] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [x] Integration tests added - [x] Documentation added - [x] Errors have helpful link attached, see `contributing.md`
vercel · Feb 4, 2022 · 61ea8ef · 61ea8ef
1 parent 06b2630
commit 61ea8ef
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 8 deletions.
diff --git a/errors/manifest.json b/errors/manifest.json
@@ -481,8 +481,12 @@
           "path": "/errors/no-script-in-document-page.md"
         },
         {
-          "title": "script-in-head-component",
-          "path": "/errors/no-script-in-head-component.md"
+          "title": "script-component-in-head-component",
+          "path": "/errors/no-script-component-in-head-component.md"
+        },
+        {
+          "title": "script-tags-in-head-component",
+          "path": "/errors/no-script-tags-in-head-component.md"
         },
         {
           "title": "max-custom-routes-reached",

diff --git a/errors/no-script-in-head-component.md → .../no-script-component-in-head-component.md b/errors/no-script-in-head-component.md → .../no-script-component-in-head-component.md
diff --git a/errors/no-script-tags-in-head-component.md b/errors/no-script-tags-in-head-component.md
@@ -0,0 +1,36 @@
+# No Script Tags In Head Component
+
+### Why This Error Occurred
+
+A `<script>` tag was added using the `next/head` component.
+
+We don't recommend this pattern because it will potentially break when used with Suspense and/or streaming. In these contexts, `next/head` tags aren't:
+
+- guaranteed to be included in the initial SSR response, so loading could be delayed until client-side rendering, regressing performance.
+
+- loaded in any particular order. The order that the app's Suspense boundaries resolve will determine the loading order of your scripts.
+
+### Possible Ways to Fix It
+
+#### Script component
+
+Use the Script component with the right loading strategy to defer loading of the script until necessary. You can see the possible strategies [here](https://nextjs.org/docs/basic-features/script/.)
+
+```jsx
+import Script from 'next/script'
+
+const Home = () => {
+  return (
+    <div class="container">
+      <Script src="https://third-party-script.js"></Script>
+      <div>Home Page</div>
+    </div>
+  )
+}
+
+export default Home
+```
+
+### Useful Links
+
+- [Script component docs](https://nextjs.org/docs/basic-features/script/)
diff --git a/packages/eslint-plugin-next/lib/index.js b/packages/eslint-plugin-next/lib/index.js
@@ -14,7 +14,7 @@ module.exports = {
     'no-document-import-in-page': require('./rules/no-document-import-in-page'),
     'no-head-import-in-document': require('./rules/no-head-import-in-document'),
     'no-script-in-document': require('./rules/no-script-in-document'),
-    'no-script-in-head': require('./rules/no-script-in-head'),
+    'no-script-component-in-head': require('./rules/no-script-component-in-head'),
     'no-server-import-in-page': require('./rules/no-server-import-in-page'),
     'no-typos': require('./rules/no-typos'),
     'no-duplicate-head': require('./rules/no-duplicate-head'),
@@ -40,7 +40,7 @@ module.exports = {
         '@next/next/no-document-import-in-page': 2,
         '@next/next/no-head-import-in-document': 2,
         '@next/next/no-script-in-document': 2,
-        '@next/next/no-script-in-head': 2,
+        '@next/next/no-script-component-in-head': 2,
         '@next/next/no-server-import-in-page': 2,
         '@next/next/no-typos': 1,
         '@next/next/no-duplicate-head': 2,

diff --git a/...lugin-next/lib/rules/no-script-in-head.js → .../lib/rules/no-script-component-in-head.js b/...lugin-next/lib/rules/no-script-in-head.js → .../lib/rules/no-script-component-in-head.js
@@ -3,7 +3,7 @@ module.exports = {
     docs: {
       description: 'Disallow using next/script inside the next/head component',
       recommended: true,
-      url: 'https://nextjs.org/docs/messages/no-script-in-head-component',
+      url: 'https://nextjs.org/docs/messages/no-script-component-in-head-component',
     },
   },
   create: function (context) {
@@ -43,7 +43,7 @@ module.exports = {
           context.report({
             node,
             message:
-              "next/script shouldn't be used inside next/head. See: https://nextjs.org/docs/messages/no-script-in-head-component",
+              "next/script shouldn't be used inside next/head. See: https://nextjs.org/docs/messages/no-script-component-in-head-component",
           })
         }
       },

diff --git a/packages/next/shared/lib/head.tsx b/packages/next/shared/lib/head.tsx
@@ -161,6 +161,12 @@ function reduceComponents(
           return React.cloneElement(c, newProps)
         }
       }
+      // TODO(kara): warn for stylesheets as well as scripts
+      if (process.env.NODE_ENV === 'development' && c.type === 'script') {
+        console.warn(
+          `Do not add <script> tags using next/head. Use next/script instead. \nSee more info here: https://nextjs.org/docs/messages/no-script-tags-in-head-component`
+        )
+      }
       return React.cloneElement(c, { key })
     })
 }

diff --git a/test/integration/client-navigation/test/index.test.js b/test/integration/client-navigation/test/index.test.js
@@ -1391,6 +1391,29 @@ describe('Client Navigation', () => {
       }
     })
 
+    it('should warn when scripts are in head', async () => {
+      let browser
+      try {
+        browser = await webdriver(context.appPort, '/head')
+
+        await browser.waitForElementByCss('h1')
+        await waitFor(2000)
+        const browserLogs = await browser.log('browser')
+        let found = false
+        browserLogs.forEach((log) => {
+          console.log('log.message', log.message)
+          if (log.message.includes('Use next/script instead')) {
+            found = true
+          }
+        })
+        expect(found).toEqual(true)
+      } finally {
+        if (browser) {
+          await browser.close()
+        }
+      }
+    })
+
     it('should update head during client routing', async () => {
       let browser
       try {

diff --git a/...int-plugin-next/no-script-in-head.test.ts → ...-next/no-script-component-in-head.test.ts b/...int-plugin-next/no-script-in-head.test.ts → ...-next/no-script-component-in-head.test.ts
@@ -1,4 +1,4 @@
-import rule from '@next/eslint-plugin-next/lib/rules/no-script-in-head'
+import rule from '@next/eslint-plugin-next/lib/rules/no-script-component-in-head'
 import { RuleTester } from 'eslint'
 ;(RuleTester as any).setDefaultConfig({
   parserOptions: {
@@ -44,7 +44,7 @@ ruleTester.run('no-script-in-head', rule, {
       errors: [
         {
           message:
-            "next/script shouldn't be used inside next/head. See: https://nextjs.org/docs/messages/no-script-in-head-component",
+            "next/script shouldn't be used inside next/head. See: https://nextjs.org/docs/messages/no-script-component-in-head-component",
         },
       ],
     },