From f563940f69a95d9792e4c8f533611e3ae2671a08 Mon Sep 17 00:00:00 2001 From: Adam Jones Date: Mon, 15 Apr 2024 23:01:50 +0100 Subject: [PATCH] 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 Co-authored-by: Sam Ko Co-authored-by: JJ Kasper --- packages/next/src/client/head-manager.ts | 26 +------- packages/next/src/client/script.tsx | 21 +------ .../src/client/set-attributes-from-props.ts | 59 +++++++++++++++++++ .../client-navigation/fixture/pages/head.js | 4 +- .../client-navigation/fixture/pages/script.js | 11 ++++ .../{test-async.js => test-async-false.js} | 0 .../fixture/public/test-async-true.js | 0 .../pages-dir/client-navigation/index.test.ts | 48 +++++++++++++++ .../pages-dir/client-navigation/rendering.ts | 3 +- tsec-exemptions.json | 1 + 10 files changed, 128 insertions(+), 45 deletions(-) create mode 100644 packages/next/src/client/set-attributes-from-props.ts create mode 100644 test/development/pages-dir/client-navigation/fixture/pages/script.js rename test/development/pages-dir/client-navigation/fixture/public/{test-async.js => test-async-false.js} (100%) create mode 100644 test/development/pages-dir/client-navigation/fixture/public/test-async-true.js diff --git a/packages/next/src/client/head-manager.ts b/packages/next/src/client/head-manager.ts index 5d9bc83565..8d477e46e9 100644 --- a/packages/next/src/client/head-manager.ts +++ b/packages/next/src/client/head-manager.ts @@ -1,30 +1,8 @@ -export const DOMAttributeNames: Record = { - 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) { diff --git a/packages/next/src/client/script.tsx b/packages/next/src/client/script.tsx index 32f8ea719b..834dc83e80 100644 --- a/packages/next/src/client/script.tsx +++ b/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 { */ 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') diff --git a/packages/next/src/client/set-attributes-from-props.ts b/packages/next/src/client/set-attributes-from-props.ts new file mode 100644 index 0000000000..f5141a57d6 --- /dev/null +++ b/packages/next/src/client/set-attributes-from-props.ts @@ -0,0 +1,59 @@ +const DOMAttributeNames: Record = { + 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) + } + } +} diff --git a/test/development/pages-dir/client-navigation/fixture/pages/head.js b/test/development/pages-dir/client-navigation/fixture/pages/head.js index bd50dddcc5..a143355259 100644 --- a/test/development/pages-dir/client-navigation/fixture/pages/head.js +++ b/test/development/pages-dir/client-navigation/fixture/pages/head.js @@ -116,7 +116,9 @@ export default () => ( {/* this should not execute twice on the client */} - + + {/* this should have async set to false on the client */} + {/* this should not execute twice on the client (intentionally sets defer to `yas` to test boolean coercion) */} diff --git a/test/development/pages-dir/client-navigation/fixture/pages/script.js b/test/development/pages-dir/client-navigation/fixture/pages/script.js new file mode 100644 index 0000000000..7e3558850f --- /dev/null +++ b/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 () => ( +
+

I am a page to test next/script

+