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

domdomegg · 2023-07-20T13:42:02Z

Summary

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.

timneutkens

Can you add integration tests for these changes?

domdomegg · 2023-08-06T00:11:27Z

Thanks @timneutkens, have added.

timneutkens · 2023-08-09T07:35:22Z

test/integration/client-navigation/test/index.test.js

@@ -1748,6 +1748,50 @@ describe('Client Navigation', () => {
    expect(value).toBe(false)
  })

+  it.each([true, false])(


Suggested change

it.each([true, false])(

it.each(['true', 'false'])(

I think this will break the assertion on line 1759 (because "false" != false)

(similarly for other comment)

Yeah, just want to make sure -${bool} doesn't break in the future. Maybe you can change that one to JSON.stringify

Sure, I've wrapped it with JSON.stringify.

timneutkens · 2023-08-09T07:35:39Z

test/integration/client-navigation/test/index.test.js

+    }
+  )
+
+  it.each([true, false])(


Suggested change

it.each([true, false])(

it.each(['true', 'false'])(

domdomegg · 2023-08-29T13:04:43Z

@timneutkens, can you take another look now that the comments have been addressed, or get someone else on the team to?

domdomegg · 2023-09-03T08:43:57Z

@balazsorban44 can you advise on next steps for getting this reviewed?

ijjk · 2023-09-04T07:33:33Z

Allow CI Workflow Run

approve CI run for commit: 848510f

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

timneutkens · 2023-09-04T07:52:35Z

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 { setBasicAttributesFromProps } from './head-manager'


Can you move this function into a separate file? The way it's set up now is that it will end up pulling in head-manager in App Router too.

After that the PR looks good to land to me.

ijjk · 2023-09-04T07:57:16Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
buildDuration	13.8s	13.8s	N/A
buildDurationCached	7.4s	6.1s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +5.7 kB
nextStartRea..uration (ms)	406ms	387ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
2453-HASH.js gzip	31.4 kB	31.4 kB	N/A
3304.HASH.js gzip	181 B	181 B	✓
3f784ff6-HASH.js gzip	53.7 kB	53.7 kB	✓
8299-HASH.js gzip	5.04 kB	5.04 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	241 B	N/A
main-HASH.js gzip	32.2 kB	32.3 kB	N/A
webpack-HASH.js gzip	1.68 kB	1.68 kB	N/A
Overall change	99 kB	99 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
_app-HASH.js gzip	196 B	197 B	N/A
_error-HASH.js gzip	184 B	184 B	✓
amp-HASH.js gzip	505 B	505 B	✓
css-HASH.js gzip	324 B	325 B	N/A
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	258 B	258 B	✓
head-HASH.js gzip	352 B	352 B	✓
hooks-HASH.js gzip	370 B	371 B	N/A
image-HASH.js gzip	4.21 kB	4.21 kB	✓
index-HASH.js gzip	259 B	259 B	✓
link-HASH.js gzip	2.67 kB	2.67 kB	N/A
routerDirect..HASH.js gzip	314 B	312 B	N/A
script-HASH.js gzip	386 B	386 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	6.57 kB	6.57 kB	✓

Client Build Manifests

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
_buildManifest.js gzip	481 B	484 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
index.html gzip	529 B	529 B	✓
link.html gzip	542 B	542 B	✓
withRouter.html gzip	525 B	523 B	N/A
Overall change	1.07 kB	1.07 kB	✓

Edge SSR bundle Size

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
edge-ssr.js gzip	95.4 kB	95.4 kB	N/A
page.js gzip	3.06 kB	3.06 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
middleware-b..fest.js gzip	627 B	625 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	25.5 kB	25.5 kB	N/A
edge-runtime..pack.js gzip	839 B	839 B	✓
Overall change	990 B	990 B	✓

Next Runtimes

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
app-page-exp...dev.js gzip	170 kB	170 kB	✓
app-page-exp..prod.js gzip	97 kB	97 kB	✓
app-page-tur..prod.js gzip	98.8 kB	98.8 kB	✓
app-page-tur..prod.js gzip	93 kB	93 kB	✓
app-page.run...dev.js gzip	144 kB	144 kB	✓
app-page.run..prod.js gzip	91.5 kB	91.5 kB	✓
app-route-ex...dev.js gzip	21.4 kB	21.4 kB	✓
app-route-ex..prod.js gzip	15.2 kB	15.2 kB	✓
app-route-tu..prod.js gzip	15.2 kB	15.2 kB	✓
app-route-tu..prod.js gzip	14.9 kB	14.9 kB	✓
app-route.ru...dev.js gzip	21.1 kB	21.1 kB	✓
app-route.ru..prod.js gzip	14.9 kB	14.9 kB	✓
pages-api-tu..prod.js gzip	9.55 kB	9.55 kB	✓
pages-api.ru...dev.js gzip	9.82 kB	9.82 kB	✓
pages-api.ru..prod.js gzip	9.55 kB	9.55 kB	✓
pages-turbo...prod.js gzip	22.5 kB	22.5 kB	✓
pages.runtim...dev.js gzip	23.1 kB	23.1 kB	✓
pages.runtim..prod.js gzip	22.4 kB	22.4 kB	✓
server.runti..prod.js gzip	51.1 kB	51.1 kB	✓
Overall change	945 kB	945 kB	✓

build cache Overall increase ⚠️

	vercel/next.js canary	domdomegg/next.js fix-52935	Change
0.pack gzip	1.58 MB	1.59 MB	⚠️ +7.95 kB
index.pack gzip	106 kB	106 kB	N/A
Overall change	1.58 MB	1.59 MB	⚠️ +7.95 kB

Diff details

Diff for middleware.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Commit: 9256f1f

timneutkens · 2023-09-11T12:21:49Z

If CI passes please don't click update branch then it should auto-land 🙏

domdomegg · 2023-10-01T15:05:04Z

Gah, foiled by a typo. Have fixed merge conflicts again but this devleopment process is highly frustrating.

Is there a way that either it's possible for me to:

trigger CI myself, e.g. without having to wait a week for approval etc.?
run all the tests run in CI locally easily (I've reviewed https://github.com/vercel/next.js/blob/canary/contributing.md, but it often gets caught on different types of tests or linting problems, which aren't explained)

In the interim, can you approve again so CI runs?

domdomegg · 2023-10-10T20:48:44Z

Hi @timneutkens could I bump on running CI again?

ozzyonfire · 2024-03-15T14:59:48Z

This looks like this will solve the exact issue I was facing, but it is unable to be merged because of a failing test. When I run this tests locally (and prettier) everything works. Do we need to kick off another CI?

Thanks - looking forward to this getting merged in!

domdomegg · 2024-03-16T20:15:55Z

@samcx Can you approve the CI run on this PR please?

samcx · 2024-03-17T19:52:45Z

@domdomegg Thanks for the ping! Taking a look :frog-eyes:

domdomegg · 2024-04-05T00:05:55Z

Whoo, all the CI is passing! 🎉

@ijjk can we merge this now?

ijjk · 2024-04-05T03:21:04Z

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

+
+    const attr = DOMAttributeNames[p] || p.toLowerCase()
+
+    if (el.tagName === 'SCRIPT' && isBooleanScriptAttribute(attr)) {


Suggested change

if (el.tagName === 'SCRIPT' && isBooleanScriptAttribute(attr)) {

if (el.tagName === 'SCRIPT' && isBooleanScriptAttribute(attr) && value !== 'false') {

Couldn't we avoid setting the initial value here for the 'false' case and then that should remove the need for the setAttribute(attr, ''); removeAttribute(attr) condition below?

Unfortunately, this will have the same problem that this PR is trying to fix:

false, this will enter this if condition and set the attribute to false, which HTML casts to "false" which is interpreted as true (non-empty string)

"false", this will enter the else condition and set the attribute to "false" which is interpreted as true (non-empty string)

This also breaks for script elements, which when inserted by JavaScript automatically default to async=true. So for async=false we need to do this setAttribute/removeAttribute trick explicitly. See https://html.spec.whatwg.org/multipage/scripting.html#script-force-async

See vercel/next.js#52939

next/script: Correctly apply async and defer props

9c6cf4c

ijjk added the type: next label Jul 20, 2023

Fix build

9556454

domdomegg mentioned this pull request Jul 20, 2023

Docs: following developing.md results in 'turbo: command not found' #52943

Closed

Standardize head and script props rendering

01482a5

domdomegg marked this pull request as ready for review July 20, 2023 16:17

domdomegg requested review from timneutkens, ijjk, shuding, huozhi and feedthejim as code owners July 20, 2023 16:17

timneutkens requested changes Jul 27, 2023

View reviewed changes

domdomegg added 2 commits August 5, 2023 23:59

Merge branch 'canary' into fix-52935

515e187

Add tests, harmonize next/head and next/script code

130f0f0

domdomegg requested a review from ztanner as a code owner August 5, 2023 23:55

ijjk added the area: tests label Aug 5, 2023

timneutkens reviewed Aug 9, 2023

View reviewed changes

Address review feedback

a98c6ab

domdomegg requested a review from timneutkens August 18, 2023 08:18

domdomegg added 2 commits August 18, 2023 09:18

Merge branch 'canary' into fix-52935

e9c26d0

Merge branch 'canary' into fix-52935

bb5628a

timneutkens added the CI approved Approve running CI for fork label Sep 4, 2023

timneutkens reviewed Sep 4, 2023

View reviewed changes

Merge branch 'canary' into fix-52935

b071027

domdomegg added 2 commits September 4, 2023 17:43

Merge branch 'canary' into fix-52935

610548f

Fix CI

b85a8f1

domdomegg dismissed timneutkens’s stale review via b85a8f1 September 4, 2023 16:55

Merge branch 'canary' of github.com:vercel/next.js into fix-52935

ac22e77

timneutkens previously approved these changes Sep 11, 2023

View reviewed changes

Fix typo

b693bf0

domdomegg dismissed timneutkens’s stale review via b693bf0 October 1, 2023 14:43

Merge remote-tracking branch 'upstream/canary' into fix-52935

2267650

Merge branch 'canary' into fix-52935

c9e638f

Merge branch 'canary' into fix-52935

3fa006d

samcx removed the CI approved Approve running CI for fork label Mar 9, 2024

Merge branch 'canary' into fix-52935

1ba1141

domdomegg added 2 commits March 16, 2024 13:01

Merge branch 'canary' into fix-52935

194cc9f

Fix lint issue

848510f

Merge branch 'canary' into fix-52935

97a8e66

domdomegg and others added 2 commits April 4, 2024 22:01

Merge branch 'canary' into fix-52935

132a960

Merge branch 'canary' into fix-52935

9256f1f

ijjk reviewed Apr 5, 2024

View reviewed changes

ijjk approved these changes Apr 15, 2024

View reviewed changes

ijjk merged commit f563940 into vercel:canary Apr 15, 2024
73 checks passed

domdomegg added a commit to bluedotimpact/meet that referenced this pull request Apr 19, 2024

Upgrade to next canary

5dcb106

See vercel/next.js#52939

github-actions bot added the locked label Apr 30, 2024

github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

domdomegg commented Jul 20, 2023 •

edited

Loading

timneutkens left a comment

domdomegg commented Aug 6, 2023

timneutkens Aug 9, 2023

domdomegg Aug 9, 2023

timneutkens Aug 9, 2023

domdomegg Aug 9, 2023

timneutkens Aug 9, 2023

domdomegg commented Aug 29, 2023

domdomegg commented Sep 3, 2023

ijjk commented Sep 4, 2023 •

edited

Loading

timneutkens Sep 4, 2023

ijjk commented Sep 4, 2023 •

edited

Loading

timneutkens commented Sep 11, 2023

domdomegg commented Oct 1, 2023

domdomegg commented Oct 10, 2023

ozzyonfire commented Mar 15, 2024

domdomegg commented Mar 16, 2024

samcx commented Mar 17, 2024

domdomegg commented Apr 5, 2024 •

edited

Loading

ijjk Apr 5, 2024

domdomegg Apr 6, 2024


		const attr = DOMAttributeNames[p] \|\| p.toLowerCase()

		if (el.tagName === 'SCRIPT' && isBooleanScriptAttribute(attr)) {

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

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

Conversation

domdomegg commented Jul 20, 2023 • edited Loading

Summary

Next steps

timneutkens left a comment

Choose a reason for hiding this comment

domdomegg commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domdomegg commented Aug 29, 2023

domdomegg commented Sep 3, 2023

ijjk commented Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

ijjk commented Sep 4, 2023 • edited Loading

Stats from current PR

timneutkens commented Sep 11, 2023

domdomegg commented Oct 1, 2023

domdomegg commented Oct 10, 2023

ozzyonfire commented Mar 15, 2024

domdomegg commented Mar 16, 2024

samcx commented Mar 17, 2024

domdomegg commented Apr 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domdomegg commented Jul 20, 2023 •

edited

Loading

ijjk commented Sep 4, 2023 •

edited

Loading

ijjk commented Sep 4, 2023 •

edited

Loading

domdomegg commented Apr 5, 2024 •

edited

Loading