Changes to the beforeInteractive strategy to make it work for streami…

…ng (#31936) Changes to the beforeInteractive strategy to make it work for streaming Splitting `beforeInteractive` into two strategies `beforeInteractive` at the _document level and `beforePageRender` for page level <Scripts>
vercel · Apr 21, 2022 · 0441f81 · 0441f81
1 parent 3b8e2f6
commit 0441f81
Show file tree

Hide file tree

Showing 17 changed files with 328 additions and 207 deletions.
diff --git a/docs/basic-features/script.md b/docs/basic-features/script.md
@@ -73,13 +73,30 @@ There are three different loading strategies that can be used:
 
 #### beforeInteractive
 
-Scripts that load with the `beforeInteractive` strategy are injected into the initial HTML from the server and run before self-bundled JavaScript is executed. This strategy should be used for any critical scripts that need to be fetched and executed before the page is interactive.
+Scripts that load with the `beforeInteractive` strategy are injected into the initial HTML from the server and run before self-bundled JavaScript is executed. This strategy should be used for any critical scripts that need to be fetched and executed before any page becomes interactive. This strategy only works inside **\_document.js** and is designed to load scripts that are needed by the entire site (i.e. the script will load when any page in the application has been loaded server-side).
+
+The reason `beforeInteractive` was designed to work only inside `\_document.js` is to support streaming and Suspense functionality. Outside of the `_document`, it's not possible to guarantee the timing or ordering of `beforeInteractive` scripts.
 
 ```jsx
-<Script
-  src="https://cdn.jsdelivr.net/npm/cookieconsent@3/build/cookieconsent.min.js"
-  strategy="beforeInteractive"
-/>
+// In _document.js
+import { Html, Head, Main, NextScript } from 'next/document'
+import Script from 'next/script'
+
+export default function Document() {
+  return (
+    <Html>
+      <Head />
+      <body>
+        <Main />
+        <NextScript />
+        <Script
+          src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js"
+          strategy="beforeInteractive"
+        ></Script>
+      </body>
+    </Html>
+  )
+}
 ```
 
 Examples of scripts that should be loaded as soon as possible with this strategy include:

diff --git a/errors/manifest.json b/errors/manifest.json
@@ -562,6 +562,10 @@
           "title": "script-in-document-page",
           "path": "/errors/no-script-in-document-page.md"
         },
+        {
+          "title": "before-interactive-script-outside-document",
+          "path": "/errors/no-before-interactive-script-outside-document.md"
+        },
         {
           "title": "script-component-in-head-component",
           "path": "/errors/no-script-component-in-head-component.md"

diff --git a/errors/no-before-interactive-script-outside-document.md b/errors/no-before-interactive-script-outside-document.md
@@ -0,0 +1,33 @@
+# beforeInteractive Script component outside \_document.js
+
+#### Why This Error Occurred
+
+You can't use the `next/script` component with the `beforeInteractive` strategy outside the `_document.js` page. That's because `beforeInteractive` strategy only works inside **\_document.js** and is designed to load scripts that are needed by the entire site (i.e. the script will load when any page in the application has been loaded server-side).
+
+#### Possible Ways to Fix It
+
+If you want a global script, move the script inside `_document.js` page.
+
+```jsx
+// In _document.js
+import { Html, Head, Main, NextScript } from 'next/document'
+import Script from 'next/script'
+
+export default function Document() {
+  return (
+    <Html>
+      <Head />
+      <body>
+        <Main />
+        <NextScript />
+        <Script
+          src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js"
+          strategy="beforeInteractive"
+        ></Script>
+      </body>
+    </Html>
+  )
+}
+```
+
+- [next-script](https://nextjs.org/docs/basic-features/script#usage)
diff --git a/packages/eslint-plugin-next/lib/index.js b/packages/eslint-plugin-next/lib/index.js
@@ -13,13 +13,13 @@ module.exports = {
     'link-passhref': require('./rules/link-passhref'),
     '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-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'),
     'inline-script-id': require('./rules/inline-script-id'),
     'next-script-for-ga': require('./rules/next-script-for-ga'),
+    'no-before-interactive-script-outside-document': require('./rules/no-before-interactive-script-outside-document'),
     'no-assign-module-variable': require('./rules/no-assign-module-variable'),
   },
   configs: {
@@ -40,12 +40,12 @@ module.exports = {
         '@next/next/next-script-for-ga': 1,
         '@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-component-in-head': 2,
         '@next/next/no-server-import-in-page': 2,
         '@next/next/no-typos': 1,
         '@next/next/no-duplicate-head': 2,
         '@next/next/inline-script-id': 2,
+        '@next/next/no-before-interactive-script-outside-document': 1,
         '@next/next/no-assign-module-variable': 2,
       },
     },

diff --git a/packages/eslint-plugin-next/lib/rules/no-before-interactive-script-outside-document.js b/packages/eslint-plugin-next/lib/rules/no-before-interactive-script-outside-document.js
@@ -0,0 +1,51 @@
+const path = require('path')
+
+module.exports = {
+  meta: {
+    docs: {
+      description:
+        'Disallow using next/script beforeInteractive strategy outside the next/_document component',
+      recommended: true,
+      url: 'https://nextjs.org/docs/messages/no-before-interactive-script-outside-document',
+    },
+  },
+  create: function (context) {
+    let scriptImportName = null
+
+    return {
+      'ImportDeclaration[source.value="next/script"] > ImportDefaultSpecifier'(
+        node
+      ) {
+        scriptImportName = node.local.name
+      },
+      JSXOpeningElement(node) {
+        if (!scriptImportName) {
+          return
+        }
+
+        if (node.name && node.name.name !== scriptImportName) {
+          return
+        }
+
+        const strategy = node.attributes.find(
+          (child) => child.name && child.name.name === 'strategy'
+        )
+
+        if (!strategy || strategy?.value?.value !== 'beforeInteractive') {
+          return
+        }
+
+        const document = context.getFilename().split('pages')[1]
+        if (document && path.parse(document).name.startsWith('_document')) {
+          return
+        }
+
+        context.report({
+          node,
+          message:
+            'next/script beforeInteractive strategy should only be used inside next/_document. See: https://nextjs.org/docs/messages/no-before-interactive-script-outside-document',
+        })
+      },
+    }
+  },
+}
diff --git a/packages/eslint-plugin-next/lib/rules/no-script-in-document.js b/packages/eslint-plugin-next/lib/rules/no-script-in-document.js
diff --git a/packages/next/client/script.tsx b/packages/next/client/script.tsx
@@ -108,14 +108,14 @@ const loadScript = (props: ScriptProps): void => {
   document.body.appendChild(el)
 }
 
-function handleClientScriptLoad(props: ScriptProps) {
+export function handleClientScriptLoad(props: ScriptProps) {
   const { strategy = 'afterInteractive' } = props
-  if (strategy === 'afterInteractive') {
-    loadScript(props)
-  } else if (strategy === 'lazyOnload') {
+  if (strategy === 'lazyOnload') {
     window.addEventListener('load', () => {
       requestIdleCallback(() => loadScript(props))
     })
+  } else {
+    loadScript(props)
   }
 }
 
@@ -129,8 +129,20 @@ function loadLazyScript(props: ScriptProps) {
   }
 }
 
+function addBeforeInteractiveToCache() {
+  const scripts = [
+    ...document.querySelectorAll('[data-nscript="beforeInteractive"]'),
+    ...document.querySelectorAll('[data-nscript="beforePageRender"]'),
+  ]
+  scripts.forEach((script) => {
+    const cacheKey = script.id || script.getAttribute('src')
+    LoadCache.add(cacheKey)
+  })
+}
+
 export function initScriptLoader(scriptLoaderItems: ScriptProps[]) {
   scriptLoaderItems.forEach(handleClientScriptLoad)
+  addBeforeInteractiveToCache()
 }
 
 function Script(props: ScriptProps): JSX.Element | null {

diff --git a/packages/next/server/render.tsx b/packages/next/server/render.tsx
@@ -586,6 +586,7 @@ export async function renderToHTML(
     App.getInitialProps === (App as any).origGetInitialProps
 
   const hasPageGetInitialProps = !!(Component as any)?.getInitialProps
+  const hasPageScripts = (Component as any)?.unstable_scriptLoader
 
   const pageIsDynamic = isDynamicRoute(pathname)
 
@@ -774,6 +775,14 @@ export async function renderToHTML(
 
   let head: JSX.Element[] = defaultHead(inAmpMode)
 
+  let initialScripts: any = {}
+  if (hasPageScripts) {
+    initialScripts.beforeInteractive = []
+      .concat(hasPageScripts())
+      .filter((script: any) => script.props.strategy === 'beforeInteractive')
+      .map((script: any) => script.props)
+  }
+
   let scriptLoader: any = {}
   const nextExport =
     !isSSG && (renderOpts.nextExport || (dev && (isAutoExport || isFallback)))
@@ -823,7 +832,7 @@ export async function renderToHTML(
               updateScripts: (scripts) => {
                 scriptLoader = scripts
               },
-              scripts: {},
+              scripts: initialScripts,
               mountedInstances: new Set(),
             }}
           >

diff --git a/packages/next/shared/lib/router/router.ts b/packages/next/shared/lib/router/router.ts
@@ -16,6 +16,7 @@ import {
   isAssetError,
   markAssetError,
 } from '../../../client/route-loader'
+import { handleClientScriptLoad } from '../../../client/script'
 import isError, { getProperError } from '../../../lib/is-error'
 import { denormalizePagePath } from '../../../server/denormalize-page-path'
 import { normalizeLocalePath } from '../i18n/normalize-locale-path'
@@ -1275,6 +1276,15 @@ export default class Router implements BaseRouter {
       )
       let { error, props, __N_SSG, __N_SSP } = routeInfo
 
+      const component: any = routeInfo.Component
+      if (component && component.unstable_scriptLoader) {
+        const scripts = [].concat(component.unstable_scriptLoader())
+
+        scripts.forEach((script: any) => {
+          handleClientScriptLoad(script.props)
+        })
+      }
+
       // handle redirect on client-transition
       if ((__N_SSG || __N_SSP) && props) {
         if (props.pageProps && props.pageProps.__N_REDIRECT) {

diff --git a/test/e2e/next-script-worker-strategy/index.test.ts b/test/e2e/next-script-worker-strategy/index.test.ts
@@ -12,7 +12,7 @@ describe('experimental.nextScriptWorkers: false with no Partytown dependency', (
       files: {
         'pages/index.js': `
           import Script from 'next/script'
-        
+
           export default function Page() {
             return (
               <>
@@ -68,7 +68,7 @@ describe('experimental.nextScriptWorkers: true with required Partytown dependenc
       files: {
         'pages/index.js': `
           import Script from 'next/script'
-        
+
           export default function Page() {
             return (
               <>
@@ -123,7 +123,7 @@ describe('experimental.nextScriptWorkers: true with required Partytown dependenc
         `document.querySelectorAll('script[type="text/partytown"]').length`
       )
 
-      expect(predefinedWorkerScripts).toEqual(1)
+      expect(predefinedWorkerScripts).toBeGreaterThan(0)
 
       await waitFor(1000)
 
@@ -132,7 +132,7 @@ describe('experimental.nextScriptWorkers: true with required Partytown dependenc
         `document.querySelectorAll('script[type="text/partytown-x"]').length`
       )
 
-      expect(processedWorkerScripts).toEqual(1)
+      expect(processedWorkerScripts).toBeGreaterThan(0)
     } finally {
       if (browser) await browser.close()
     }
@@ -183,7 +183,7 @@ describe('experimental.nextScriptWorkers: true with config override', () => {
         `,
         'pages/index.js': `
           import Script from 'next/script'
-        
+
           export default function Page() {
             return (
               <>

diff --git a/test/integration/script-loader/base/pages/_app.js b/test/integration/script-loader/base/pages/_app.js
@@ -15,11 +15,6 @@ function MyApp({ Component, pageProps }) {
         src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=documentLazyOnload"
         strategy="lazyOnload"
       />
-      <Script
-        id="documentBeforeInteractive"
-        src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=documentBeforeInteractive"
-        strategy="beforeInteractive"
-      />
       <Component {...pageProps} />
     </>
   )