Skip to content
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
SukkaW opened this issue Aug 28, 2022 · 1 comment · Fixed by #40541
Closed
1 task done

next/script is not react strict mode resilient (not concurrent rendering safe) #40025

SukkaW opened this issue Aug 28, 2022 · 1 comment · Fixed by #40541
Labels
bug Issue was opened via the bug report template. Script (next/script) Related to Next.js Script Optimization.

Comments

@SukkaW
Copy link
Contributor

SukkaW commented Aug 28, 2022

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

    Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 22.1.0: Mon Aug 15 20:06:46 PDT 2022; root:xnu-8792.40.29.161.2~1/RELEASE_X86_64
    Binaries:
      Node: 16.16.0
      npm: 8.18.0
      Yarn: 1.22.18
      pnpm: 7.9.5
    Relevant packages:
      next: 12.2.6-canary.5
      eslint-config-next: 12.2.5
      react: 18.2.0
      react-dom: 18.2.0

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 with reactStrictMode enabled, the script with beforeInteractive or worker could load more than once. The onLoad could be called more than once. The onReady 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 (when if (strategy === 'beforeInteractive' || strategy === 'worker') {), which breaks the fundermental rule of React.

if (strategy === 'beforeInteractive' || strategy === 'worker') {
if (updateScripts) {
scripts[strategy] = (scripts[strategy] || []).concat([
{
id,
src,
onLoad,
onReady,
onError,
...restProps,
},
])
updateScripts(scripts)
} else if (getIsSsr && getIsSsr()) {
// Script has already loaded during SSR
LoadCache.add(id || src)
} else if (getIsSsr && !getIsSsr()) {
loadScript(props)
}
}

Expected Behavior

Whenever the strict mode is enabled or not, whenever the next/script is wrapped in <OffScreen /> or not, whatever the strategy is:

Remote scripts

  • script should only be loaded once
  • onLoad should only be called once, upon loaded
  • onReady should be called when the script is first loaded, or when the next/script is re-mounted

Inline scripts

  • onLoad should never be called
  • onReady should be called whenever the next/script is mounted

Link to reproduction

46b8959 (#40002)

https://github.com/vercel/next.js/runs/8049268564?check_suite_focus=true

To Reproduce

  • Clone the Next.js repo
  • Add a next.config.js with reactStrictMode: true under test/integration/script-loader/base directory
  • Find test/integration/script-loader/test/index.test.js, replace bootApp (which will test in production mode) with launchApp (which will test in dev mode)
  • pnpm run testonly --test-path-pattern test/integration/script-loader/test/index.test.js
  • Multiple existing test cases fail to pass
@SukkaW SukkaW added the bug Issue was opened via the bug report template. label Aug 28, 2022
@SukkaW SukkaW changed the title next/script is not react strict mode resilient next/script is not react strict mode resilient (not concurrent rendering safe) Aug 28, 2022
@balazsorban44 balazsorban44 added Script (next/script) Related to Next.js Script Optimization. React 18 labels Sep 1, 2022
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 <houssein.djirdeh@gmail.com>
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 <houssein.djirdeh@gmail.com>
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
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`
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2022
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants