next/script: make onLoad concurrent rendering resilient

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 <OffScreen /> 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])
 

test/integration/script-loader/base/pages/page4.js

@@ -1,33 +1,45 @@
+/* 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'
 
 const Page = () => {
   return (
     <div class="container">
+      <div id="onload-div-1" />
+      <Link href="/page9">Page 9</Link>
       <Script
         src={url}
         id="script1"
         onLoad={() => {
-          // eslint-disable-next-line no-undef
-          document.getElementById('text').textContent += _.repeat('a', 3)
+          document.getElementById('onload-div-1').textContent += _.repeat(
+            'a',
+            3
+          )
         }}
       ></Script>
       <Script
         src={url}
         id="script2"
         onLoad={() => {
           // eslint-disable-next-line no-undef
-          document.getElementById('text').textContent += _.repeat('b', 3)
+          document.getElementById('onload-div-1').textContent += _.repeat(
+            'b',
+            3
+          )
         }}
       ></Script>
       <Script
         src={url}
         id="script3"
         onLoad={() => {
           // eslint-disable-next-line no-undef
-          document.getElementById('text').textContent += _.repeat('c', 3)
+          document.getElementById('onload-div-1').textContent += _.repeat(
+            'c',
+            3
+          )
         }}
       ></Script>
     </div>

test/integration/script-loader/base/pages/page9.js

@@ -3,6 +3,7 @@ import Link from 'next/link'
 const Page = () => {
   return (
     <>
+      <Link href="/page4">Page 4</Link>
       <Link href="/page8">Page 8</Link>
     </>
   )

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)