Skip to content

[Script] Allow next/script to be placed in _document body #37894

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

Merged
kodiakhq merged 4 commits into from
Jun 27, 2022
Merged

[Script] Allow next/script to be placed in _document body #37894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8fa014f
modifes how document includes beforeInteractive scripts by including …
housseindjirdeh Jun 21, 2022
3ad4d44
updates per review comments
housseindjirdeh Jun 24, 2022
bbf8b26
adds note
housseindjirdeh Jun 27, 2022
d5f7b73
Merge branch 'canary' into bug/#37741
kodiakhq[bot] Jun 27, 2022
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
2 changes: 2 additions & 0 deletions docs/basic-features/script.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ export default function Document() {
}
```

> **Note**: Scripts with `beforeInteractive` will always be injected inside the `head` of the HTML document regardless of where it's placed in `_document.js`.

Examples of scripts that should be loaded as soon as possible with this strategy include:

- Bot detectors
Expand Down
99 changes: 62 additions & 37 deletions packages/next/pages/_document.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
DocumentInitialProps,
DocumentProps,
DocumentType,
NEXT_DATA,
} from '../shared/lib/utils'
import { BuildManifest, getPageFiles } from '../server/get-page-files'
import { cleanAmpPath } from '../server/utils'
Expand Down Expand Up @@ -87,6 +88,59 @@ function hasComponentProps(child: any): child is React.ReactElement {
return !!child && !!child.props
}

function handleDocumentScriptLoaderItems(
scriptLoader: { beforeInteractive?: any[] },
__NEXT_DATA__: NEXT_DATA,
props: any
): void {
if (!props.children) return

const scriptLoaderItems: ScriptProps[] = []

const children = Array.isArray(props.children)
? props.children
: [props.children]

const headChildren = children.find(
(child: React.ReactElement) => child.type === Head
)?.props?.children
const bodyChildren = children.find(
(child: React.ReactElement) => child.type === 'body'
)?.props?.children

// Scripts with beforeInteractive can be placed inside Head or <body> so children of both needs to be traversed
const combinedChildren = [
...(Array.isArray(headChildren) ? headChildren : [headChildren]),
...(Array.isArray(bodyChildren) ? bodyChildren : [bodyChildren]),
]

React.Children.forEach(combinedChildren, (child: any) => {
if (!child) return

if (child.type === Script) {
Copy link

@vespaiach vespaiach Jun 29, 2022

Choose a reason for hiding this comment

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

Is this intended that only Script tags are allow here?

I had my code to check whether a certain env variables set or not before including Script tag and it didn't work. Here my code sample:

class MyDocument extends Document {
    render() {
        return (
            <Html lang="en" className="scroll-smooth">
                <Head>
                    <link rel="stylesheet" href="/main.css" />
                    {process.env.NEXT_PUBLIC_GTAG_ID && (
                        <>
                            <Script
                                strategy="beforeInteractive"
                                src={`https://www.googletagmanager.com/gtag/js?id=${process.env.NEXT_PUBLIC_GTAG_ID}`}
                            />
                            <Script
                                strategy="beforeInteractive"
                                dangerouslySetInnerHTML={{
                                    __html: `
                                        window.dataLayer = window.dataLayer || [];
                                        function gtag(){dataLayer.push(arguments);}
                                        gtag('js', new Date());
                                        gtag('config', '${process.env.NEXT_PUBLIC_GTAG_ID}');
                                    `,
                                }}
                            />
                        </>
                    )}
                </Head>
                <body className="bg-white text-gray-500 antialiased dark:bg-gray-900 dark:text-white">
                    <Main />
                    <NextScript />
                </body>
            </Html>
        );
    }
}

Because there were two Script tags so I wrapped them with a React.Fragment tag.

Question:

  • Can we recursively check all the Script tags here?

Copy link
Contributor Author

@housseindjirdeh housseindjirdeh Jul 1, 2022

Choose a reason for hiding this comment

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

Thanks for spotting this. Not sure about having it work recursively, but It should work when wrapped in at least one level of <Fragment> since that's the expected behavior for other elements in <Head> .

Can you be open a new issue for this? I'll submit a patch to fix, but you can remove the fragment as a workaround for now (sorry for the inconvenience):

<Head>
  <link rel="stylesheet" href="/main.css" />
  {process.env.NEXT_PUBLIC_GTAG_ID && (
    <Script
      strategy="beforeInteractive"
      src={`https://www.googletagmanager.com/gtag/js?id=${process.env.NEXT_PUBLIC_GTAG_ID}`}
    />
  )}
  {process.env.NEXT_PUBLIC_GTAG_ID && (
    <Script
      strategy="beforeInteractive"
      dangerouslySetInnerHTML={{
        __html: `
            window.dataLayer = window.dataLayer || [];
            function gtag(){dataLayer.push(arguments);}
            gtag('js', new Date());
            gtag('config', '${process.env.NEXT_PUBLIC_GTAG_ID}');
        `,
      }}
    />
  )}
</Head>;

Copy link

@vespaiach vespaiach Jul 1, 2022

Choose a reason for hiding this comment

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

#38242

if (child.props.strategy === 'beforeInteractive') {
scriptLoader.beforeInteractive = (
scriptLoader.beforeInteractive || []
).concat([
{
...child.props,
},
])
return
} else if (
['lazyOnload', 'afterInteractive', 'worker'].includes(
child.props.strategy
)
) {
scriptLoaderItems.push(child.props)
return
}
}
})

__NEXT_DATA__.scriptLoader = scriptLoaderItems
}

function getPreNextWorkerScripts(context: HtmlProps, props: OriginProps) {
const { assetPrefix, scriptLoader, crossOrigin, nextScriptWorkers } = context

Expand Down Expand Up @@ -340,9 +394,16 @@ export function Html(
HTMLHtmlElement
>
) {
const { inAmpMode, docComponentsRendered, locale } = useContext(HtmlContext)
const {
inAmpMode,
docComponentsRendered,
locale,
scriptLoader,
__NEXT_DATA__,
} = useContext(HtmlContext)

docComponentsRendered.Html = true
handleDocumentScriptLoaderItems(scriptLoader, __NEXT_DATA__, props)

return (
<html
Expand Down Expand Up @@ -605,40 +666,6 @@ export class Head extends Component<HeadProps> {
return getPolyfillScripts(this.context, this.props)
}

handleDocumentScriptLoaderItems(children: React.ReactNode): ReactNode[] {
const { scriptLoader } = this.context
const scriptLoaderItems: ScriptProps[] = []
const filteredChildren: ReactNode[] = []

React.Children.forEach(children, (child: any) => {
if (child.type === Script) {
if (child.props.strategy === 'beforeInteractive') {
scriptLoader.beforeInteractive = (
scriptLoader.beforeInteractive || []
).concat([
{
...child.props,
},
])
return
} else if (
['lazyOnload', 'afterInteractive', 'worker'].includes(
child.props.strategy
)
) {
scriptLoaderItems.push(child.props)
return
}
}

filteredChildren.push(child)
})

this.context.__NEXT_DATA__.scriptLoader = scriptLoaderItems

return filteredChildren
}

makeStylesheetInert(node: ReactNode): ReactNode[] {
return React.Children.map(node, (c: any) => {
if (
Expand Down Expand Up @@ -740,8 +767,6 @@ export class Head extends Component<HeadProps> {
children = this.makeStylesheetInert(children)
}

children = this.handleDocumentScriptLoaderItems(children)

let hasAmphtmlRel = false
let hasCanonicalRel = false

Expand Down
65 changes: 62 additions & 3 deletions test/e2e/next-script/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { NextInstance } from 'test/lib/next-modes/base'
import { BrowserInterface } from 'test/lib/browsers/base'
import { check } from 'next-test-utils'

describe('beforeInteractive', () => {
describe('beforeInteractive in document Head', () => {
let next: NextInstance

beforeAll(async () => {
Expand All @@ -31,9 +31,67 @@ describe('beforeInteractive', () => {
)
}
`,
'pages/index.js': `
'pages/index.js': `
export default function Home() {
return (
<>
<p>Home page</p>
</>
)
}
`,
},
dependencies: {
react: '17.0.2',
'react-dom': '17.0.2',
},
})
})
afterAll(() => next.destroy())

it('Script is injected server-side', async () => {
let browser: BrowserInterface

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

const script = await browser.eval(
`document.querySelector('script[data-nscript="beforeInteractive"]')`
)
expect(script).not.toBeNull()
} finally {
if (browser) await browser.close()
}
})
})

describe('beforeInteractive in document body', () => {
let next: NextInstance

beforeAll(async () => {
next = await createNext({
files: {
'pages/_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
Copy link
Member

@ijjk ijjk Jun 24, 2022

Choose a reason for hiding this comment

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

Is it expected that a next/script beforeInteractive tag defined in the body would be moved the head automatically? This may confuse users that do want it to be loaded in the body unless we warn for this case or keep it in place.

Screen Shot 2022-06-24 at 11 21 31 AM

Copy link
Contributor Author

@housseindjirdeh housseindjirdeh Jun 24, 2022

Choose a reason for hiding this comment

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

Yeah the idea behind beforeInteractive is that it should be used sparingly to load a script before any other script on the page (including Next.js chunks). That was always the behavior before we begin enforcing users to place it in _document.js, regardless of which component/page it was placed in. I worry that we'll change its intrinsic behavior if we begin to inject scripts in body if the user starts placing things there.

However, I understand that it can be confusing for users to place a script in body but not see it outputted that way in the DOM. Should I add a console and/or ESLint warning for this? (e.g. "beforeInteractive scripts will always be inserted into the document head. If you would like to inject a script into the body instead, do this instead...")

CC @janicklas-ralph for his thoughts too.

Copy link
Member

@ijjk ijjk Jun 24, 2022

Choose a reason for hiding this comment

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

I see, I think it does make sense that next/script will move the scripts depending on where we find leads to the best performance so might not need a warning/eslint rule and instead could just be a note in the docs.

Copy link
Contributor Author

@housseindjirdeh housseindjirdeh Jun 27, 2022

Choose a reason for hiding this comment

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

Added a note 🙏

src="https://www.google-analytics.com/analytics.js"
strategy="beforeInteractive"
/>
</body>
</Html>
)
}
`,
'pages/index.js': `
export default function Home() {
return (
<>
Expand All @@ -60,6 +118,7 @@ describe('beforeInteractive', () => {
const script = await browser.eval(
`document.querySelector('script[data-nscript="beforeInteractive"]')`
)

expect(script).not.toBeNull()
} finally {
if (browser) await browser.close()
Expand Down
4 changes: 4 additions & 0 deletions test/integration/script-loader/base/pages/_document.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export default function Document() {
<body>
<Main />
<NextScript />
<Script
src="https://www.google-analytics.com/analytics.js?a=scriptBeforeInteractive"
strategy="beforeInteractive"
></Script>
<div id="text" />
</body>
</Html>
Expand Down
4 changes: 2 additions & 2 deletions test/integration/script-loader/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ describe('Next.js Script - Primary Strategies', () => {
let documentBIScripts = await browser.elementsByCss(
'[src$="scriptBeforeInteractive"]'
)
expect(documentBIScripts.length).toBe(1)
expect(documentBIScripts.length).toBe(2)

await browser.waitForElementByCss('[href="/page1"]')
await browser.click('[href="/page1"]')
Expand All @@ -170,7 +170,7 @@ describe('Next.js Script - Primary Strategies', () => {
documentBIScripts = await browser.elementsByCss(
'[src$="scriptBeforeInteractive"]'
)
expect(documentBIScripts.length).toBe(1)
expect(documentBIScripts.length).toBe(2)
} finally {
if (browser) await browser.close()
}
Expand Down