From 9c6cf4cff8c6af6f13132f369f4f85a94b5e90b1 Mon Sep 17 00:00:00 2001 From: Adam Jones Date: Thu, 20 Jul 2023 14:30:04 +0100 Subject: [PATCH 1/9] next/script: Correctly apply async and defer props --- packages/next/src/client/script.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/next/src/client/script.tsx b/packages/next/src/client/script.tsx index a4b2c6dcd3184..5e9f0567106c0 100644 --- a/packages/next/src/client/script.tsx +++ b/packages/next/src/client/script.tsx @@ -142,6 +142,9 @@ const loadScript = (props: ScriptProps): void => { afterLoad() } else if (src) { el.src = src + el.async = props.async + el.defer = props.defer + // do not add cacheKey into LoadCache for remote script here // cacheKey will be added to LoadCache when it is actually loaded (see loadPromise above) From 955645416a84f3de4a21d0c3c26a5ec771eaf1a1 Mon Sep 17 00:00:00 2001 From: Adam Jones Date: Thu, 20 Jul 2023 15:04:00 +0100 Subject: [PATCH 2/9] Fix build --- packages/next/src/client/script.tsx | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/next/src/client/script.tsx b/packages/next/src/client/script.tsx index 5e9f0567106c0..0b48f183c4d38 100644 --- a/packages/next/src/client/script.tsx +++ b/packages/next/src/client/script.tsx @@ -79,6 +79,8 @@ const loadScript = (props: ScriptProps): void => { strategy = 'afterInteractive', onError, stylesheets, + async, + defer, } = props const cacheKey = id || src @@ -142,8 +144,15 @@ const loadScript = (props: ScriptProps): void => { afterLoad() } else if (src) { el.src = src - el.async = props.async - el.defer = props.defer + + // We must apply these manually because setAttribute does not update the way the scripts are actually loaded + // See https://github.com/vercel/next.js/pull/52939 + if (async !== undefined) { + el.async = async; + } + if (defer !== undefined) { + el.defer = defer; + } // do not add cacheKey into LoadCache for remote script here // cacheKey will be added to LoadCache when it is actually loaded (see loadPromise above) From 01482a55b96679e04a8ac8dd56085fbe2e236e18 Mon Sep 17 00:00:00 2001 From: Adam Jones Date: Thu, 20 Jul 2023 16:30:49 +0100 Subject: [PATCH 3/9] Standardize head and script props rendering --- packages/next/src/client/head-manager.ts | 24 +++++++++++++------- packages/next/src/client/script.tsx | 29 +++++++++++------------- 2 files changed, 29 insertions(+), 24 deletions(-) diff --git a/packages/next/src/client/head-manager.ts b/packages/next/src/client/head-manager.ts index 5d9bc835653af..3f47990f706a5 100644 --- a/packages/next/src/client/head-manager.ts +++ b/packages/next/src/client/head-manager.ts @@ -6,23 +6,31 @@ export const DOMAttributeNames: Record = { noModule: 'noModule', } +export function isBooleanScriptAttribute(attr: string): attr is "async" | "defer" | "noModule" { + return ["async", "defer", "noModule"].includes(attr) +} + 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 + const value = props[p] // we don't render undefined props to the DOM - if (props[p] === undefined) continue + if (value === 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]) + el.setAttribute(attr, 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) + // NB: We must still 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 + if (value === false || (type === "script" && isBooleanScriptAttribute(attr) && (!value || value === "false"))) { + el.removeAttribute(attr) } } diff --git a/packages/next/src/client/script.tsx b/packages/next/src/client/script.tsx index 0b48f183c4d38..1129f9990729d 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 { ScriptHTMLAttributes } from 'react' import { HeadManagerContext } from '../shared/lib/head-manager-context' -import { DOMAttributeNames } from './head-manager' +import { DOMAttributeNames, isBooleanScriptAttribute } from './head-manager' import { requestIdleCallback } from './request-idle-callback' const ScriptCache = new Map() @@ -79,8 +79,6 @@ const loadScript = (props: ScriptProps): void => { strategy = 'afterInteractive', onError, stylesheets, - async, - defer, } = props const cacheKey = id || src @@ -144,29 +142,28 @@ const loadScript = (props: ScriptProps): void => { afterLoad() } else if (src) { el.src = src - - // We must apply these manually because setAttribute does not update the way the scripts are actually loaded - // See https://github.com/vercel/next.js/pull/52939 - if (async !== undefined) { - el.async = async; - } - if (defer !== undefined) { - el.defer = defer; - } - // do not add cacheKey into LoadCache for remote script here // cacheKey will be added to LoadCache when it is actually loaded (see loadPromise above) ScriptCache.set(src, loadPromise) } - for (const [k, value] of Object.entries(props)) { - if (value === undefined || ignoreProps.includes(k)) { + for (const [p, value] of Object.entries(props)) { + // we don't render undefined props to the DOM + if (value === undefined || ignoreProps.includes(p)) { continue } - const attr = DOMAttributeNames[k] || k.toLowerCase() + const attr = DOMAttributeNames[p] || p.toLowerCase() el.setAttribute(attr, 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) + // NB: We must still 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 + if (value === false || (isBooleanScriptAttribute(attr) && (!value || value === "false"))) { + el.removeAttribute(attr) + } } if (strategy === 'worker') { From 130f0f0a0cfeb19e2a640ebf5288d36f8ee583c5 Mon Sep 17 00:00:00 2001 From: Adam Jones Date: Sun, 6 Aug 2023 00:55:05 +0100 Subject: [PATCH 4/9] Add tests, harmonize next/head and next/script code --- packages/next/src/client/head-manager.ts | 50 +++++++++++++++---- packages/next/src/client/script.tsx | 30 +---------- .../client-navigation/pages/head.js | 4 +- .../client-navigation/pages/script.js | 11 ++++ .../{test-async.js => test-async-false.js} | 0 .../public/test-async-true.js | 0 .../client-navigation/test/index.test.js | 44 ++++++++++++++++ 7 files changed, 99 insertions(+), 40 deletions(-) create mode 100644 test/integration/client-navigation/pages/script.js rename test/integration/client-navigation/public/{test-async.js => test-async-false.js} (100%) create mode 100644 test/integration/client-navigation/public/test-async-true.js diff --git a/packages/next/src/client/head-manager.ts b/packages/next/src/client/head-manager.ts index 3f47990f706a5..5d50a02917583 100644 --- a/packages/next/src/client/head-manager.ts +++ b/packages/next/src/client/head-manager.ts @@ -6,16 +6,26 @@ export const DOMAttributeNames: Record = { noModule: 'noModule', } -export function isBooleanScriptAttribute(attr: string): attr is "async" | "defer" | "noModule" { - return ["async", "defer", "noModule"].includes(attr) +function isBooleanScriptAttribute( + attr: string +): attr is 'async' | 'defer' | 'noModule' { + return ['async', 'defer', 'noModule'].includes(attr) } -function reactElementToDOM({ type, props }: JSX.Element): HTMLElement { - const el: HTMLElement = document.createElement(type) - for (const p in props) { +const ignoreProps = [ + 'onLoad', + 'onReady', + 'dangerouslySetInnerHTML', + 'children', + 'onError', + 'strategy', + 'stylesheets', +] + +export function setBasicAttributesFromProps(el: HTMLElement, props: object) { + for (const [p, value] of Object.entries(props)) { if (!props.hasOwnProperty(p)) continue - if (p === 'children' || p === 'dangerouslySetInnerHTML') continue - const value = props[p] + if (ignoreProps.includes(p)) continue // we don't render undefined props to the DOM if (value === undefined) { @@ -23,16 +33,34 @@ function reactElementToDOM({ type, props }: JSX.Element): HTMLElement { } const attr = DOMAttributeNames[p] || p.toLowerCase() - el.setAttribute(attr, value) + + 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) - // NB: We must still 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 - if (value === false || (type === "script" && isBooleanScriptAttribute(attr) && (!value || value === "false"))) { + 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) } } +} + +function reactElementToDOM({ type, props }: JSX.Element): HTMLElement { + const el: HTMLElement = document.createElement(type) + setBasicAttributesFromProps(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 1129f9990729d..38487e1c296f2 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 { ScriptHTMLAttributes } from 'react' import { HeadManagerContext } from '../shared/lib/head-manager-context' -import { DOMAttributeNames, isBooleanScriptAttribute } from './head-manager' +import { setBasicAttributesFromProps } from './head-manager' 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,23 +138,7 @@ const loadScript = (props: ScriptProps): void => { ScriptCache.set(src, loadPromise) } - for (const [p, value] of Object.entries(props)) { - // we don't render undefined props to the DOM - if (value === undefined || ignoreProps.includes(p)) { - continue - } - - const attr = DOMAttributeNames[p] || p.toLowerCase() - el.setAttribute(attr, 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) - // NB: We must still 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 - if (value === false || (isBooleanScriptAttribute(attr) && (!value || value === "false"))) { - el.removeAttribute(attr) - } - } + setBasicAttributesFromProps(el, props) if (strategy === 'worker') { el.setAttribute('type', 'text/partytown') diff --git a/test/integration/client-navigation/pages/head.js b/test/integration/client-navigation/pages/head.js index bd50dddcc54d4..a143355259fa2 100644 --- a/test/integration/client-navigation/pages/head.js +++ b/test/integration/client-navigation/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/integration/client-navigation/pages/script.js b/test/integration/client-navigation/pages/script.js new file mode 100644 index 0000000000000..7e3558850fd92 --- /dev/null +++ b/test/integration/client-navigation/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

+