next/script: simplify logic and update tests (vercel#40026)

The PR is the first step toward fixing vercel#40025. The PR makes the `script-loader` integration test run on both dev and production modes.

Some existing test cases are skipped in dev mode because corresponding features are not strict mode resilient and thus will fail. They will be included in dev mode tests in the future.

The PR also merges some duplicated logic in `next/script`, and adds a detailed comment about how `onReady` works.

In the next PR, I will try to fix `onLoad` being called more than once under strict mode.

Co-authored-by: Houssein Djirdeh <houssein.djirdeh@gmail.com>

packages/next/client/script.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect, useContext } from 'react'
+import React, { useEffect, useContext, useRef } from 'react'
 import { ScriptHTMLAttributes } from 'react'
 import { HeadManagerContext } from '../shared/lib/head-manager-context'
 import { DOMAttributeNames } from './head-manager'
@@ -57,21 +57,25 @@ const loadScript = (props: ScriptProps): void => {
     return
   }
 
+  /** Execute after the script first loaded */
+  const afterLoad = () => {
+    // Run onReady for the first time after load event
+    if (onReady) {
+      onReady()
+    }
+    // add cacheKey to LoadCache when load successfully
+    LoadCache.add(cacheKey)
+  }
+
   const el = document.createElement('script')
 
   const loadPromise = new Promise<void>((resolve, reject) => {
     el.addEventListener('load', function (e) {
-      // add cacheKey to LoadCache when load successfully
-      LoadCache.add(cacheKey)
-
       resolve()
       if (onLoad) {
         onLoad.call(this, e)
       }
-      // Run onReady for the first time after load event
-      if (onReady) {
-        onReady()
-      }
+      afterLoad()
     })
     el.addEventListener('error', function (e) {
       reject(e)
@@ -85,8 +89,7 @@ const loadScript = (props: ScriptProps): void => {
   if (dangerouslySetInnerHTML) {
     el.innerHTML = dangerouslySetInnerHTML.__html || ''
 
-    // add cacheKey to LoadCache for inline script
-    LoadCache.add(cacheKey)
+    afterLoad()
   } else if (children) {
     el.textContent =
       typeof children === 'string'
@@ -95,8 +98,7 @@ const loadScript = (props: ScriptProps): void => {
         ? children.join('')
         : ''
 
-    // add cacheKey to LoadCache for inline script
-    LoadCache.add(cacheKey)
+    afterLoad()
   } else if (src) {
     el.src = src
     // do not add cacheKey into LoadCache for remote script here
@@ -174,12 +176,42 @@ function Script(props: ScriptProps): JSX.Element | null {
   // Context is available only during SSR
   const { updateScripts, scripts, getIsSsr } = useContext(HeadManagerContext)
 
+  /**
+   * - First mount:
+   *   1. The useEffect for onReady executes
+   *   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
+   *   [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
+   *
+   * - Second mount:
+   *   1. The useEffect for onReady executes
+   *   2. hasOnReadyEffectCalled.current is false, but the script has already loaded (found in LoadCache)
+   *      onReady is called, set hasOnReadyEffectCalled.current to true
+   *   3. The useEffect for loadScript executes
+   *   4. The script is already loaded, loadScript bails out
+   *   [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, loadScript will bail out
+   */
+  const hasOnReadyEffectCalled = useRef(false)
+
   useEffect(() => {
     const cacheKey = id || src
+    if (!hasOnReadyEffectCalled.current) {
+      // Run onReady if script has loaded before but component is re-mounted
+      if (onReady && cacheKey && LoadCache.has(cacheKey)) {
+        onReady()
+      }
 
-    // Run onReady if script has loaded before but component is re-mounted
-    if (onReady && cacheKey && LoadCache.has(cacheKey)) {
-      onReady()
+      hasOnReadyEffectCalled.current = true
     }
   }, [onReady, id, src])
 

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

@@ -1,11 +1,6 @@
 import Script from 'next/script'
 import Link from 'next/link'
 
-if (typeof window !== 'undefined') {
-  window.remoteScriptsOnReadyCalls ??= 0
-  window.inlineScriptsOnReadyCalls ??= 0
-}
-
 const Page = () => {
   return (
     <div className="container">
@@ -14,13 +9,15 @@ const Page = () => {
       <Script
         src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.1/jquery.min.js"
         onReady={() => {
+          window.remoteScriptsOnReadyCalls ??= 0
           window.remoteScriptsOnReadyCalls++
         }}
       />
       <Script
         id="i-am-an-inline-script-that-has-on-ready"
         dangerouslySetInnerHTML={{ __html: 'console.log("inline script!")' }}
         onReady={() => {
+          window.inlineScriptsOnReadyCalls ??= 0
           window.inlineScriptsOnReadyCalls++
         }}
       />