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.

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

Adds inline script functionality to next/script for worker and beforeInteractive strategies #36364

Merged
merged 5 commits into from
Apr 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 1 addition & 4 deletions docs/basic-features/script.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,10 +243,7 @@ Or by using the `dangerouslySetInnerHTML` property:
/>
```

There are two limitations to be aware of when using the Script component for inline scripts:

- Only the `afterInteractive` and `lazyOnload` strategies can be used. The `beforeInteractive` loading strategy injects the contents of an external script into the initial HTML response. Inline scripts already do this, which is why **the `beforeInteractive` strategy cannot be used with inline scripts.**
- An `id` attribute must be defined in order for Next.js to track and optimize the script
The `id` property is required for **inline scripts** in order for Next.js to track and optimize the script.

### Executing Code After Loading (`onLoad`)

Expand Down
38 changes: 38 additions & 0 deletions errors/invalid-script.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Invalid Script

#### Why This Error Occurred

Somewhere in your application, you are using the `next/script` component without including an inline script or `src` attribute.

#### Possible Ways to Fix It

Look for any usage of the `next/script` component and make sure that `src` is provided or an inline script is used.

**Compatible `src` attribute**

```jsx
<Script src="https://example.com/analytics.js" />
```

**Compatible inline script with curly braces**

```jsx
<Script id="show-banner">
{`document.getElementById('banner').classList.remove('hidden')`}
</Script>
```

**Compatible inline script with `dangerouslySetInnerHtml`**

```jsx
<Script
id="show-banner"
dangerouslySetInnerHTML={{
__html: `document.getElementById('banner').classList.remove('hidden')`,
}}
/>
```

### Useful Links

- [Script Component in Documentation](https://nextjs.org/docs/basic-features/script)
4 changes: 4 additions & 0 deletions errors/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,10 @@
{
"title": "no-assign-module-variable",
"path": "/errors/no-assign-module-variable.md"
},
{
"title": "invalid-script",
"path": "/errors/invalid-script.md"
}
]
}
Expand Down
1 change: 0 additions & 1 deletion packages/next/client/script.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ function Script(props: ScriptProps): JSX.Element | null {
const {
src = '',
onLoad = () => {},
dangerouslySetInnerHTML,
strategy = 'afterInteractive',
onError,
...restProps
Expand Down
97 changes: 88 additions & 9 deletions packages/next/pages/_document.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -121,12 +121,54 @@ function getPreNextWorkerScripts(context: HtmlProps, props: OriginProps) {
}}
/>
{(scriptLoader.worker || []).map((file: ScriptProps, index: number) => {
const { strategy, ...scriptProps } = file
const {
strategy,
src,
children: scriptChildren,
dangerouslySetInnerHTML,
...scriptProps
} = file

let srcProps: {
src?: string
dangerouslySetInnerHTML?: {
__html: string
}
} = {}

if (src) {
// Use external src if provided
srcProps.src = src
} else if (
dangerouslySetInnerHTML &&
dangerouslySetInnerHTML.__html
) {
// Embed inline script if provided with dangerouslySetInnerHTML
srcProps.dangerouslySetInnerHTML = {
__html: dangerouslySetInnerHTML.__html,
}
} else if (scriptChildren) {
// Embed inline script if provided with children
srcProps.dangerouslySetInnerHTML = {
__html:
typeof scriptChildren === 'string'
? scriptChildren
: Array.isArray(scriptChildren)
? scriptChildren.join('')
: '',
}
styfle marked this conversation as resolved.
Show resolved Hide resolved
} else {
throw new Error(
'Invalid usage of next/script. Did you forget to include a src attribute or an inline script? https://nextjs.org/docs/messages/invalid-script'
)
}

return (
<script
{...srcProps}
{...scriptProps}
type="text/partytown"
key={scriptProps.src || index}
key={src || index}
nonce={props.nonce}
data-nscript="worker"
crossOrigin={props.crossOrigin || crossOrigin}
Expand All @@ -137,9 +179,7 @@ function getPreNextWorkerScripts(context: HtmlProps, props: OriginProps) {
)
} catch (err) {
if (isError(err) && err.code !== 'MODULE_NOT_FOUND') {
console.warn(
`Warning: Partytown could not be instantiated in your application due to an error. ${err.message}`
)
console.warn(`Warning: ${err.message}`)
}
return null
}
Expand All @@ -150,8 +190,9 @@ function getPreNextScripts(context: HtmlProps, props: OriginProps) {

const webWorkerScripts = getPreNextWorkerScripts(context, props)

const beforeInteractiveScripts = (scriptLoader.beforeInteractive || []).map(
(file: ScriptProps, index: number) => {
const beforeInteractiveScripts = (scriptLoader.beforeInteractive || [])
.filter((script) => script.src)
.map((file: ScriptProps, index: number) => {
const { strategy, ...scriptProps } = file
return (
<script
Expand All @@ -163,8 +204,7 @@ function getPreNextScripts(context: HtmlProps, props: OriginProps) {
crossOrigin={props.crossOrigin || crossOrigin}
/>
)
}
)
})

return (
<>
Expand Down Expand Up @@ -500,6 +540,44 @@ export class Head extends Component<
]
}

getBeforeInteractiveInlineScripts() {
Copy link
Collaborator Author

@housseindjirdeh housseindjirdeh Apr 25, 2022

Choose a reason for hiding this comment

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

@janicklas-ralph Would it make more sense if I just move the src or dangerouslyInnerHtml logic I wrote in getPreNextWorkerScripts to a separate function and reuse that for beforeInteractive scripts? We wouldn't need this separate getter if we did that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. For beforeInteractive we have two functions, one for inline and one for regular scripts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noted. As discussed, the two functions are needed here because of ordering so I'll leave it as is :)

const { scriptLoader } = this.context
const { nonce, crossOrigin } = this.props

return (scriptLoader.beforeInteractive || [])
.filter(
(script) =>
!script.src && (script.dangerouslySetInnerHTML || script.children)
)
.map((file: ScriptProps, index: number) => {
const { strategy, children, dangerouslySetInnerHTML, ...scriptProps } =
file
let html = ''

if (dangerouslySetInnerHTML && dangerouslySetInnerHTML.__html) {
html = dangerouslySetInnerHTML.__html
} else if (children) {
html =
typeof children === 'string'
? children
: Array.isArray(children)
? children.join('')
: ''
}

return (
<script
{...scriptProps}
dangerouslySetInnerHTML={{ __html: html }}
key={scriptProps.id || index}
nonce={nonce}
data-nscript="beforeInteractive"
crossOrigin={crossOrigin || process.env.__NEXT_CROSS_ORIGIN}
/>
)
})
}

getDynamicChunks(files: DocumentFiles) {
return getDynamicChunks(this.context, this.props, files)
}
Expand Down Expand Up @@ -782,6 +860,7 @@ export class Head extends Component<
href={canonicalBase + getAmpPath(ampPath, dangerousAsPath)}
/>
)}
{this.getBeforeInteractiveInlineScripts()}
{!optimizeCss && this.getCssLinks(files)}
{!optimizeCss && <noscript data-n-css={this.props.nonce ?? ''} />}

Expand Down
98 changes: 97 additions & 1 deletion test/e2e/next-script-worker-strategy/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('experimental.nextScriptWorkers: false with no Partytown dependency', (
})
})

describe('experimental.nextScriptWorkers: true with required Partytown dependency', () => {
describe('experimental.nextScriptWorkers: true with required Partytown dependency for external script', () => {
let next: NextInstance

beforeAll(async () => {
Expand Down Expand Up @@ -139,6 +139,102 @@ describe('experimental.nextScriptWorkers: true with required Partytown dependenc
})
})

describe('experimental.nextScriptWorkers: true with required Partytown dependency for inline script', () => {
const createNextApp = async (script) =>
await createNext({
nextConfig: {
experimental: {
nextScriptWorkers: true,
},
dependencies: {
react: '17',
'react-dom': '17',
},
},
files: {
'pages/index.js': `
import Script from 'next/script'
export default function Page() {
return (
<>
${script}
<div id="text" />
</>
)
}
`,
},
dependencies: {
'@builder.io/partytown': '0.4.2',
},
})

it('Inline worker script through children is modified by Partytown to execute on a worker thread', async () => {
let next: NextInstance
let browser: BrowserInterface

next = await createNextApp(
`<Script id="inline-script" strategy="worker">{"document.getElementById('text').textContent += 'abc'"}</Script>`
)

try {
browser = await webdriver(next.url, '/')

const predefinedWorkerScripts = await browser.eval(
`document.querySelectorAll('script[type="text/partytown"]').length`
)
expect(predefinedWorkerScripts).toEqual(1)

await waitFor(1000)

// Partytown modifes type to "text/partytown-x" after it has been executed in the web worker
const processedWorkerScripts = await browser.eval(
`document.querySelectorAll('script[type="text/partytown-x"]').length`
)
expect(processedWorkerScripts).toEqual(1)

const text = await browser.elementById('text').text()
expect(text).toBe('abc')
} finally {
if (browser) await browser.close()
await next.destroy()
}
})

it('Inline worker script through dangerouslySetInnerHtml is modified by Partytown to execute on a worker thread', async () => {
let next: NextInstance
let browser: BrowserInterface

next = await createNextApp(
`<Script id="inline-script" strategy="worker" dangerouslySetInnerHTML={{__html: "document.getElementById('text').textContent += 'abcd'"}}/>`
)

try {
browser = await webdriver(next.url, '/')

const predefinedWorkerScripts = await browser.eval(
`document.querySelectorAll('script[type="text/partytown"]').length`
)
expect(predefinedWorkerScripts).toEqual(1)

await waitFor(1000)

// Partytown modifes type to "text/partytown-x" after it has been executed in the web worker
const processedWorkerScripts = await browser.eval(
`document.querySelectorAll('script[type="text/partytown-x"]').length`
)
expect(processedWorkerScripts).toEqual(1)

const text = await browser.elementById('text').text()
expect(text).toBe('abcd')
} finally {
if (browser) await browser.close()
await next.destroy()
}
})
})

describe('experimental.nextScriptWorkers: true with config override', () => {
let next: NextInstance

Expand Down
7 changes: 7 additions & 0 deletions test/integration/script-loader/base/pages/_document.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ export default function Document() {
src="https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js?a=scriptBeforeInteractive"
strategy="beforeInteractive"
></Script>
<Script
id="inline-before"
strategy="beforeInteractive"
dangerouslySetInnerHTML={{
__html: `console.log('inline beforeInteractive')`,
}}
></Script>
</Head>
<body>
<Main />
Expand Down
13 changes: 13 additions & 0 deletions test/integration/script-loader/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,19 @@ describe('Next.js Script - Primary Strategies', () => {
}
})

it('priority beforeInteractive with inline script', async () => {
const html = await renderViaHTTP(appPort, '/page5')
const $ = cheerio.load(html)

const script = $('#inline-before')
expect(script.length).toBe(1)

// Script is inserted before CSS
expect(
$(`#inline-before ~ link[href^="/_next/static/css"]`).length
).toBeGreaterThan(0)
})

it('Does not duplicate inline scripts', async () => {
let browser
try {
Expand Down