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.
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
Changes to the beforeInteractive strategy to make it work for streaming #31936
Changes from 7 commits
bd0e593
74c3b90
534cdc9
5f4b871
34b1dfd
fb0c0c1
4183720
375a603
60e0b6f
7830ead
162c6b2
b9af011
1f0586d
899b506
0e2437b
a0f97b4
0a38ef9
def82bc
5a0aaf8
caa62ca
b9d293f
1b3389b
bc9c136
d6967f9
6530d68
15ea355
d83e4c5
73c282a
9660ff9
242324d
7a591e5
b370a52
99dcc1d
b6cb974
450ab3b
316f368
11fdcad
6627015
19f9ae1
1bec0e6
c6e2f2f
d8d6a4a
01346c1
beeb03a
243b0ef
e00ed11
2bc903b
04253eb
0d8e9d5
e17be39
bf11680
265c654
d292f25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -64,28 +64,73 @@ With `next/script`, you decide when to load your third-party script by using the | |||||
<Script src="https://connect.facebook.net/en_US/sdk.js" strategy="lazyOnload" /> | ||||||
``` | ||||||
|
||||||
There are three different loading strategies that can be used: | ||||||
There are four different loading strategies that can be used: | ||||||
|
||||||
- `beforeInteractive`: Load before the page is interactive | ||||||
- `beforeInteractive`: Load scripts required by the entire site before the page is interactive | ||||||
- `beforePageRender`: Load scripts required by a particular page before the page is interactive | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
janicklas-ralph marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
- `afterInteractive`: (**default**): Load immediately after the page becomes interactive | ||||||
- `lazyOnload`: Load during idle time | ||||||
|
||||||
#### 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 the page is interactive. This strategy only works inside **\_document.js** and is designed to load scripts that is needed by the entire site. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Which of the following correct?
I assume it's the first option, and if so - can we clarify that a bit here? Something along the lines of:
Suggested change
janicklas-ralph marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
```jsx | ||||||
<Script | ||||||
src="https://cdn.jsdelivr.net/npm/cookieconsent@3/build/cookieconsent.min.js" | ||||||
strategy="beforeInteractive" | ||||||
/> | ||||||
// In _document.js | ||||||
|
||||||
ijjk marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
export default class MyDocument extends Document { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use a functional |
||||||
render() { | ||||||
return ( | ||||||
<Html> | ||||||
<Head /> | ||||||
<body> | ||||||
<Main /> | ||||||
<NextScript /> | ||||||
<Script | ||||||
id="scriptBeforeInteractive" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the |
||||||
src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=scriptBeforeInteractive" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused about the
Suggested change
|
||||||
strategy="beforeInteractive" | ||||||
></Script> | ||||||
</body> | ||||||
</Html> | ||||||
) | ||||||
} | ||||||
} | ||||||
``` | ||||||
|
||||||
Examples of scripts that should be loaded as soon as possible with this strategy include: | ||||||
|
||||||
- Bot detectors | ||||||
- Cookie consent managers | ||||||
|
||||||
#### beforePageRender | ||||||
|
||||||
Scripts that load with the `beforePageRender` strategy are injected into the initial HTML from the server and run before self-bundled JavaScript is executed. This strategy is similar to `beforeInteractive` but is designed for scripts that are needed by a page and not the entire site. Syntax for adding the script is as shown below. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here, can we clarify that this Script will load only for a page that it has been used in.
Suggested change
|
||||||
|
||||||
```jsx | ||||||
import Script from 'next/script' | ||||||
|
||||||
const Page = () => { | ||||||
return ( | ||||||
<div class="container"> | ||||||
<div>page1</div> | ||||||
</div> | ||||||
) | ||||||
} | ||||||
|
||||||
Page.scriptLoader = () => { | ||||||
return ( | ||||||
<Script | ||||||
id="scriptBeforePageRender" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here, can we remove the unnecessary id?
Suggested change
|
||||||
src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=scriptBeforePageRender" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly here, can we remove the unnecessary query param?
Suggested change
|
||||||
strategy="beforePageRender" | ||||||
></Script> | ||||||
) | ||||||
} | ||||||
|
||||||
export default Page | ||||||
``` | ||||||
|
||||||
#### afterInteractive | ||||||
|
||||||
Scripts that use the `afterInteractive` strategy are injected client-side and will run after Next.js hydrates the page. This strategy should be used for scripts that do not need to load as soon as possible and can be fetched and executed immediately after the page is interactive. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ module.exports = { | |
'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-script-bi-outside-document': require('./rules/no-script-bi-outside-document'), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (feel free to ignore): I think it would be nicer to not shorten beforeInteractive to |
||
}, | ||
configs: { | ||
recommended: { | ||
|
@@ -39,12 +40,13 @@ 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-in-document': 0, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're turning off this rule entirely, we should probably just delete it completely (unless you think there's a reason why folks would like to turn it on?) |
||
'@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-script-bi-outside-document': 2, | ||
}, | ||
}, | ||
'core-web-vitals': { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
const path = require('path') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In addition to an ESLint warning, would be good to have a runtime console warning since some people don't use ESLint and this will actually break once streaming is added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @janicklas-ralph There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. I think it's OK to put the console warning in a separate PR though (I'm good with merging this one with just the ESLint warning and docs) |
||
|
||
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-script-bi-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.value && 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-script-bi-outside-document', | ||
janicklas-ralph marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}) | ||
}, | ||
} | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -547,6 +547,7 @@ export async function renderToHTML( | |
App.getInitialProps === (App as any).origGetInitialProps | ||
|
||
const hasPageGetInitialProps = !!(Component as any)?.getInitialProps | ||
const hasPageScripts = (Component as any).scriptLoader | ||
|
||
const pageIsDynamic = isDynamicRoute(pathname) | ||
|
||
|
@@ -735,6 +736,18 @@ 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' || | ||
script.props.strategy === 'beforePageRender' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove |
||
) | ||
.map((script: any) => script.props) | ||
} | ||
|
||
let scriptLoader: any = {} | ||
const nextExport = | ||
!isSSG && (renderOpts.nextExport || (dev && (isAutoExport || isFallback))) | ||
|
@@ -784,7 +797,7 @@ export async function renderToHTML( | |
updateScripts: (scripts) => { | ||
scriptLoader = scripts | ||
}, | ||
scripts: {}, | ||
scripts: initialScripts, | ||
mountedInstances: new Set(), | ||
}} | ||
> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,19 @@ import Script from 'next/script' | |
const Page = () => { | ||
return ( | ||
<div class="container"> | ||
<Script | ||
id="scriptBeforeInteractive" | ||
src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=scriptBeforeInteractive" | ||
strategy="beforeInteractive" | ||
></Script> | ||
<div>page1</div> | ||
</div> | ||
) | ||
} | ||
|
||
Page.scriptLoader = () => { | ||
return ( | ||
<Script | ||
id="scriptBeforePageRender" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably want to revert this beforePageRender test as well? |
||
src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=scriptBeforePageRender" | ||
strategy="beforePageRender" | ||
></Script> | ||
) | ||
} | ||
|
||
export default Page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.