next/script: Correctly apply async and defer props (#52939)

### Summary

Fixes #52935

`next/script` has a `Script` component that supports an `async` prop.
However, when scripts are loaded with the `async` prop set to false, the
script is loaded as if async was set to true. This may cause scripts to
execute out of order. Repro:
https://github.com/domdomegg/next-async-script-reproduction

I think this is occurring because Next uses setAttribute to set the
async and defer attributes. However, this is not a valid way to set
these properties on a script. This is because . Demo:
https://jsfiddle.net/6ktpfae1/

This PR fixes this behaviour by using removeAttribute after calling
setAttribute (rather than using setAttribute "false"). This appears to
result in correct behaviour.

Given it appears this workaround was already applied in `next/head`,
I've harmonised the code between these two.

### Next steps

I think this PR is ready for review. I acknowledge there are no test
changes, but there are no existing tests for `next/script` at all and
creating them I think would be disproportionally difficult given issues
in #52943.

---------

Co-authored-by: Tim Neutkens <tim@timneutkens.nl>
Co-authored-by: Sam Ko <sam@vercel.com>
Co-authored-by: JJ Kasper <jj@jjsweb.site>

packages/next/src/client/head-manager.ts

@@ -1,30 +1,8 @@
-export const DOMAttributeNames: Record<string, string> = {
-  acceptCharset: 'accept-charset',
-  className: 'class',
-  htmlFor: 'for',
-  httpEquiv: 'http-equiv',
-  noModule: 'noModule',
-}
+import { setAttributesFromProps } from './set-attributes-from-props'
 
 function reactElementToDOM({ type, props }: JSX.Element): HTMLElement {
   const el: HTMLElement = document.createElement(type)
-  for (const p in props) {
-    if (!props.hasOwnProperty(p)) continue
-    if (p === 'children' || p === 'dangerouslySetInnerHTML') continue
-
-    // we don't render undefined props to the DOM
-    if (props[p] === undefined) continue
-
-    const attr = DOMAttributeNames[p] || p.toLowerCase()
-    if (
-      type === 'script' &&
-      (attr === 'async' || attr === 'defer' || attr === 'noModule')
-    ) {
-      ;(el as HTMLScriptElement)[attr] = !!props[p]
-    } else {
-      el.setAttribute(attr, props[p])
-    }
-  }
+  setAttributesFromProps(el, props)
 
   const { children, dangerouslySetInnerHTML } = props
   if (dangerouslySetInnerHTML) {

packages/next/src/client/script.tsx

@@ -4,7 +4,7 @@ import ReactDOM from 'react-dom'
 import React, { useEffect, useContext, useRef } from 'react'
 import type { ScriptHTMLAttributes } from 'react'
 import { HeadManagerContext } from '../shared/lib/head-manager-context.shared-runtime'
-import { DOMAttributeNames } from './head-manager'
+import { setAttributesFromProps } from './set-attributes-from-props'
 import { requestIdleCallback } from './request-idle-callback'
 
 const ScriptCache = new Map()
@@ -25,16 +25,6 @@ export interface ScriptProps extends ScriptHTMLAttributes<HTMLScriptElement> {
  */
 export type Props = ScriptProps
 
-const ignoreProps = [
-  'onLoad',
-  'onReady',
-  'dangerouslySetInnerHTML',
-  'children',
-  'onError',
-  'strategy',
-  'stylesheets',
-]
-
 const insertStylesheets = (stylesheets: string[]) => {
   // Case 1: Styles for afterInteractive/lazyOnload with appDir injected via handleClientScriptLoad
   //
@@ -148,14 +138,7 @@ const loadScript = (props: ScriptProps): void => {
     ScriptCache.set(src, loadPromise)
   }
 
-  for (const [k, value] of Object.entries(props)) {
-    if (value === undefined || ignoreProps.includes(k)) {
-      continue
-    }
-
-    const attr = DOMAttributeNames[k] || k.toLowerCase()
-    el.setAttribute(attr, value)
-  }
+  setAttributesFromProps(el, props)
 
   if (strategy === 'worker') {
     el.setAttribute('type', 'text/partytown')

packages/next/src/client/set-attributes-from-props.ts

@@ -0,0 +1,59 @@
+const DOMAttributeNames: Record<string, string> = {
+  acceptCharset: 'accept-charset',
+  className: 'class',
+  htmlFor: 'for',
+  httpEquiv: 'http-equiv',
+  noModule: 'noModule',
+}
+
+const ignoreProps = [
+  'onLoad',
+  'onReady',
+  'dangerouslySetInnerHTML',
+  'children',
+  'onError',
+  'strategy',
+  'stylesheets',
+]
+
+function isBooleanScriptAttribute(
+  attr: string
+): attr is 'async' | 'defer' | 'noModule' {
+  return ['async', 'defer', 'noModule'].includes(attr)
+}
+
+export function setAttributesFromProps(el: HTMLElement, props: object) {
+  for (const [p, value] of Object.entries(props)) {
+    if (!props.hasOwnProperty(p)) continue
+    if (ignoreProps.includes(p)) continue
+
+    // we don't render undefined props to the DOM
+    if (value === undefined) {
+      continue
+    }
+
+    const attr = DOMAttributeNames[p] || p.toLowerCase()
+
+    if (el.tagName === 'SCRIPT' && isBooleanScriptAttribute(attr)) {
+      // Correctly assign boolean script attributes
+      // https://github.com/vercel/next.js/pull/20748
+      ;(el as HTMLScriptElement)[attr] = !!value
+    } else {
+      el.setAttribute(attr, String(value))
+    }
+
+    // Remove falsy non-zero boolean attributes so they are correctly interpreted
+    // (e.g. if we set them to false, this coerces to the string "false", which the browser interprets as true)
+    if (
+      value === false ||
+      (el.tagName === 'SCRIPT' &&
+        isBooleanScriptAttribute(attr) &&
+        (!value || value === 'false'))
+    ) {
+      // Call setAttribute before, as we need to set and unset the attribute to override force async:
+      // https://html.spec.whatwg.org/multipage/scripting.html#script-force-async
+      el.setAttribute(attr, '')
+      el.removeAttribute(attr)
+    }
+  }
+}

test/development/pages-dir/client-navigation/fixture/pages/head.js

@@ -116,7 +116,9 @@ export default () => (
       <link rel="stylesheet" href="dedupe-style.css" key="my-style" />
 
       {/* this should not execute twice on the client */}
-      <script src="/test-async.js" async></script>
+      <script src="/test-async-true.js" async></script>
+      {/* this should have async set to false on the client */}
+      <script src="/test-async-false.js" async={false}></script>
       {/* this should not execute twice on the client (intentionally sets defer to `yas` to test boolean coercion) */}
       <script src="/test-defer.js" defer="yas"></script>
 

test/development/pages-dir/client-navigation/fixture/pages/script.js

@@ -0,0 +1,11 @@
+import React from 'react'
+import Script from 'next/script'
+
+export default () => (
+  <div>
+    <h1>I am a page to test next/script</h1>
+    <Script src="/test-async-true.js" async />
+    <Script src="/test-async-false.js" async={false} />
+    <Script src="/test-defer.js" defer />
+  </div>
+)

test/development/pages-dir/client-navigation/index.test.ts

@@ -1773,6 +1773,54 @@ createNextDescribe(
       expect(value).toBe(false)
     })
 
+    it.each([true, false])(
+      'should handle boolean async prop in next/head client-side: %s',
+      async (bool) => {
+        const browser = await webdriver(next.appPort, '/head')
+        const value = await browser.eval(
+          `document.querySelector('script[src="/test-async-${JSON.stringify(
+            bool
+          )}.js"]').async`
+        )
+
+        expect(value).toBe(bool)
+      }
+    )
+
+    it.each([true, false])(
+      'should handle boolean async prop in next/script client-side: %s',
+      async (bool) => {
+        const browser = await webdriver(next.appPort, '/script')
+        const value = await browser.eval(
+          `document.querySelector('script[src="/test-async-${JSON.stringify(
+            bool
+          )}.js"]').async`
+        )
+
+        expect(value).toBe(bool)
+      }
+    )
+
+    it('should only execute async and defer scripts with next/script once', async () => {
+      let browser
+      try {
+        browser = await webdriver(next.appPort, '/script')
+
+        await browser.waitForElementByCss('h1')
+        await waitFor(2000)
+        expect(
+          Number(await browser.eval('window.__test_async_executions'))
+        ).toBe(1)
+        expect(
+          Number(await browser.eval('window.__test_defer_executions'))
+        ).toBe(1)
+      } finally {
+        if (browser) {
+          await browser.close()
+        }
+      }
+    })
+
     it('should emit routeChangeError on hash change cancel', async () => {
       const browser = await webdriver(next.appPort, '/')
 

test/development/pages-dir/client-navigation/rendering.ts

@@ -187,7 +187,8 @@ export default function (next: NextInstance, render, fetch, ctx) {
 
     test('header helper renders boolean attributes correctly children', async () => {
       const html = await render('/head')
-      expect(html).toContain('<script src="/test-async.js" async="">')
+      expect(html).toContain('<script src="/test-async-true.js" async="">')
+      expect(html).toContain('<script src="/test-async-false.js">')
       expect(html).toContain('<script src="/test-defer.js" defer="">')
     })
 

tsec-exemptions.json

@@ -8,6 +8,7 @@
   "ban-element-setattribute": [
     "packages/next/src/client/head-manager.ts",
     "packages/next/src/client/script.tsx",
+    "packages/next/src/client/set-attributes-from-props.ts",
     "packages/next/src/build/webpack/loaders/next-style-loader/runtime/injectStylesIntoLinkTag.ts",
     "packages/next/src/build/webpack/loaders/next-style-loader/runtime/injectStylesIntoStyleTag.ts",
     "packages/next/src/client/app-bootstrap.ts"