-
Notifications
You must be signed in to change notification settings - Fork 27.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
next/script
is not react strict mode resilient (not concurrent rendering safe)
#40025
Closed
1 task done
Labels
bug
Issue was opened via the bug report template.
Script (next/script)
Related to Next.js Script Optimization.
Comments
next/script
is not react strict mode resilientnext/script
is not react strict mode resilient (not concurrent rendering safe)
ijjk
pushed a commit
that referenced
this issue
Sep 2, 2022
The PR is the first step toward fixing #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 <[email protected]>
This was referenced Sep 3, 2022
atilafassina
pushed a commit
to atilafassina/next.js
that referenced
this issue
Sep 5, 2022
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 <[email protected]>
ijjk
pushed a commit
that referenced
this issue
Sep 14, 2022
Another step toward fixing #40025. Multiple `next/script` components with the same `src` may exist in the Next.js app. So the `loadScript` function will always attach the `onLoad` handler to the `loadingPromise` every time it executes. However, with strict mode (or wrapped inside the `<OffScreen />` component), the `useEffect` could execute more than once for the same `next/script` component, thus the `loadScript` for each `next/script` component could execute more than once (and `onLoad` to be attached more than once), results in `onLoad` fires more than once. The PR makes sure that for every `next/script` component mounted, the `loadScript` will always be executed only once for each of them. The corresponding `onload fires correctly` integration test case is also updated to run in dev mode. ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md`
SukkaW
added a commit
to SukkaW/next.js
that referenced
this issue
Sep 14, 2022
3 tasks
ijjk
pushed a commit
that referenced
this issue
Sep 14, 2022
…rod (#40541) Ref: #40002 #40026 #40191 Fixes #40025 This is the final step of fixing #40025. The PR migrates the rest of the `next/script` test cases to run in both dev (strict mode) and production, confirming that the `next/script` component is now completely concurrent rendering resilient and is ready for the upcoming React 18 `<OffScreen />`. ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md`
This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
bug
Issue was opened via the bug report template.
Script (next/script)
Related to Next.js Script Optimization.
Verify canary release
Provide environment information
What browser are you using? (if relevant)
No response
How are you deploying your application? (if relevant)
No response
Describe the Bug
The
next/script
is not React Strict Mode resilient. During development withreactStrictMode
enabled, the script withbeforeInteractive
orworker
could load more than once. TheonLoad
could be called more than once. TheonReady
used to be called more than once (see #39993), and I have fixed it in #40002.And there are many other hidden race conditions.
By checking the source code of
next/script
, it is clear that a side effect (loadScript
) may execute during the render phase (whenif (strategy === 'beforeInteractive' || strategy === 'worker') {
), which breaks the fundermental rule of React.next.js/packages/next/client/script.tsx
Lines 194 to 213 in e6862e4
Expected Behavior
Whenever the strict mode is enabled or not, whenever the
next/script
is wrapped in<OffScreen />
or not, whatever thestrategy
is:Remote scripts
onLoad
should only be called once, upon loadedonReady
should be called when the script is first loaded, or when thenext/script
is re-mountedInline scripts
onLoad
should never be calledonReady
should be called whenever thenext/script
is mountedLink to reproduction
46b8959
(#40002)https://github.com/vercel/next.js/runs/8049268564?check_suite_focus=true
To Reproduce
next.config.js
withreactStrictMode: true
undertest/integration/script-loader/base
directorytest/integration/script-loader/test/index.test.js
, replacebootApp
(which will test in production mode) withlaunchApp
(which will test in dev mode)pnpm run testonly --test-path-pattern test/integration/script-loader/test/index.test.js
The text was updated successfully, but these errors were encountered: