From ed3cb83c8ad8ce18bc711f9977057ba1839b98e0 Mon Sep 17 00:00:00 2001 From: Sukka Date: Wed, 14 Sep 2022 08:48:06 +0800 Subject: [PATCH] next/script: make `onLoad` concurrent rendering resilient (#40191) Another step toward fixing #40025. Multiple `next/script` components with the same `src` may exist in the Next.js app. So the `loadScript` function will always attach the `onLoad` handler to the `loadingPromise` every time it executes. However, with strict mode (or wrapped inside the `` component), the `useEffect` could execute more than once for the same `next/script` component, thus the `loadScript` for each `next/script` component could execute more than once (and `onLoad` to be attached more than once), results in `onLoad` fires more than once. The PR makes sure that for every `next/script` component mounted, the `loadScript` will always be executed only once for each of them. The corresponding `onload fires correctly` integration test case is also updated to run in dev mode. ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` --- packages/next/client/script.tsx | 24 ++++++++----- .../script-loader/base/pages/page4.js | 20 ++++++++--- .../script-loader/base/pages/page9.js | 1 + .../script-loader/test/index.test.js | 34 ++++++++++++------- 4 files changed, 55 insertions(+), 24 deletions(-) diff --git a/packages/next/client/script.tsx b/packages/next/client/script.tsx index e9ebd6a022c0..b309ffc8224c 100644 --- a/packages/next/client/script.tsx +++ b/packages/next/client/script.tsx @@ -52,7 +52,8 @@ const loadScript = (props: ScriptProps): void => { // Contents of this script are already loading/loaded if (ScriptCache.has(src)) { LoadCache.add(cacheKey) - // Execute onLoad since the script loading has begun + // It is possible that multiple `next/script` components all have same "src", but has different "onLoad" + // This is to make sure the same remote script will only load once, but "onLoad" are executed in order ScriptCache.get(src).then(onLoad, onError) return } @@ -182,12 +183,13 @@ function Script(props: ScriptProps): JSX.Element | null { * 2. hasOnReadyEffectCalled.current is false, but the script hasn't loaded yet (not in LoadCache) * onReady is skipped, set hasOnReadyEffectCalled.current to true * 3. The useEffect for loadScript executes - * Once the script is loaded, the onReady will be called by then + * 4. hasLoadScriptEffectCalled.current is false, loadScript executes + * Once the script is loaded, the onLoad and onReady will be called by then * [If strict mode is enabled / is wrapped in component] * 5. The useEffect for onReady executes again * 6. hasOnReadyEffectCalled.current is true, so entire effect is skipped * 7. The useEffect for loadScript executes again - * 8. The script is already loaded/loading, loadScript bails out + * 8. hasLoadScriptEffectCalled.current is true, so entire effect is skipped * * - Second mount: * 1. The useEffect for onReady executes @@ -199,7 +201,7 @@ function Script(props: ScriptProps): JSX.Element | null { * 5. The useEffect for onReady executes again * 6. hasOnReadyEffectCalled.current is true, so entire effect is skipped * 7. The useEffect for loadScript executes again - * 8. The script is already loaded, loadScript will bail out + * 8. hasLoadScriptEffectCalled.current is true, so entire effect is skipped */ const hasOnReadyEffectCalled = useRef(false) @@ -215,11 +217,17 @@ function Script(props: ScriptProps): JSX.Element | null { } }, [onReady, id, src]) + const hasLoadScriptEffectCalled = useRef(false) + useEffect(() => { - if (strategy === 'afterInteractive') { - loadScript(props) - } else if (strategy === 'lazyOnload') { - loadLazyScript(props) + if (!hasLoadScriptEffectCalled.current) { + if (strategy === 'afterInteractive') { + loadScript(props) + } else if (strategy === 'lazyOnload') { + loadLazyScript(props) + } + + hasLoadScriptEffectCalled.current = true } }, [props, strategy]) diff --git a/test/integration/script-loader/base/pages/page4.js b/test/integration/script-loader/base/pages/page4.js index aa1ac29358a1..118ebda1c835 100644 --- a/test/integration/script-loader/base/pages/page4.js +++ b/test/integration/script-loader/base/pages/page4.js @@ -1,4 +1,6 @@ +/* global _ */ import Script from 'next/script' +import Link from 'next/link' const url = 'https://cdnjs.cloudflare.com/ajax/libs/lodash.js/4.17.20/lodash.min.js' @@ -6,12 +8,16 @@ const url = const Page = () => { return (
+
+ Page 9
diff --git a/test/integration/script-loader/base/pages/page9.js b/test/integration/script-loader/base/pages/page9.js index 6e82d8627683..ffc9a1bd7d7a 100644 --- a/test/integration/script-loader/base/pages/page9.js +++ b/test/integration/script-loader/base/pages/page9.js @@ -3,6 +3,7 @@ import Link from 'next/link' const Page = () => { return ( <> + Page 4 Page 8 ) diff --git a/test/integration/script-loader/test/index.test.js b/test/integration/script-loader/test/index.test.js index d30692240220..f56eb4030097 100644 --- a/test/integration/script-loader/test/index.test.js +++ b/test/integration/script-loader/test/index.test.js @@ -163,21 +163,31 @@ const runTests = (isDev = false) => { }) } - if (!isDev) { - it('onload fires correctly', async () => { - let browser - try { - browser = await webdriver(appPort, '/page4') - await waitFor(3000) + it('onload fires correctly', async () => { + let browser + try { + browser = await webdriver(appPort, '/page4') + await waitFor(3000) - const text = await browser.elementById('text').text() + const text = await browser.elementById('onload-div-1').text() + expect(text).toBe('aaabbbccc') - expect(text).toBe('aaabbbccc') - } finally { - if (browser) await browser.close() - } - }) + // Navigate to different page and back + await browser.waitForElementByCss('[href="/page9"]') + await browser.click('[href="/page9"]') + await browser.waitForElementByCss('[href="/page4"]') + await browser.click('[href="/page4"]') + await browser.waitForElementByCss('#onload-div-1') + const sameText = await browser.elementById('onload-div-1').text() + // onload should only be fired once, not on sequential re-mount + expect(sameText).toBe('') + } finally { + if (browser) await browser.close() + } + }) + + if (!isDev) { it('priority beforeInteractive with inline script', async () => { const html = await renderViaHTTP(appPort, '/page5') const $ = cheerio.load(html)