Next.js includes useSyncExternalStore shim even with React 18

[gaearon]

Verify canary release

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

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 21.2.0: Sun Nov 28 20:28:41 PST 2021; root:xnu-8019.61.5~1/RELEASE_ARM64_T6000
Binaries:
  Node: 16.14.2
  npm: 8.5.0
  Yarn: 1.22.15
  pnpm: 6.11.0
Relevant packages:
  next: 12.1.7-canary.11
  react: 0.0.0-experimental-82c64e1a4-20220520
  react-dom: 0.0.0-experimental-82c64e1a4-20220520

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

This brings in an external package:

next.js/packages/next/shared/lib/loadable.js

Line 25 in 26459ef

import { useSyncExternalStore } from 'use-sync-external-store/shim'

However, React 18 already supports useSyncExternalStore directly.

Expected Behavior

Don't bundle the shim when the user is on React 18+.

To Reproduce

Create a new Next.js project.

[SukkaW]

@gaearon

use-sync-external-store package was first added to the Next.js by #36733 in order to replace use-subscription (which was added back in 2019 to add concurrent "mode" support for react-loadable, see #9026). use-sync-external-store/shim package was used to retain the compatibility with React 17.

At the time React.lazy was introduced to React 16.6, it doesn't have SSR support, that's why react-loadable was used. With React 18 new SSR architecture, react-lodable is no longer necessary for code splitting, and next/dynamic with { suspense: true } is just a thin wrapper over React.lazy for the API compatibility.

Since beta.reactjs.org is already using React 18, IMHO it is better to use React.lazy directly instead of next/dynamic. And without the usage of next/dynamic, use-sync-external-store package will not be included in the final bundle.

[gaearon]

My understanding is that next/dynamic is still providing an advantage over raw React.lazy because this makes Next.js emit some kind of a preload (or prefetch?) hint. Is that not the case?

[gaearon]

Another related aspect is the documentation. The documentation puts the Suspense version at the end and also says something about "SSR in concurrent mode" not being ready. Which I think is outdated wording (there is no "mode", and SSR works fine with it). So I think the docs need to change to emphasize the approach best for React 18 (whichever it is).

[SukkaW]

My understanding is that next/dynamic is still providing an advantage over raw React.lazy because this makes Next.js emit some kind of a preload (or prefetch?) hint. Is that not the case?

I just checked the source code of Next.js and the PR that introduces the { suspense: true } to the next/dynamic (#27611), and it seems that it is not the case, { suspense: true } just uses React.lazy directly (without react-loadable).

Maybe in the future, Next.js would add this as a feature.

Another related aspect is the documentation. The documentation puts the Suspense version at the end and also says something about "SSR in concurrent mode" not being ready. Which I think is outdated wording (there is no "mode", and SSR works fine with it). So I think the docs need to change to emphasize the approach best for React 18 (whichever it is).

The documentation does look kinda outdated and does not explicitly mention which one is preferred (React.lazy or next/dynamic) under specific cases (E.g. if React 18 is being used, or if React 17 + blocking SSR is being used). And I just assume it is because the next/dynamic was introduced back in the day when React.lazy doesn't support SSR and it uses react-lodable under the hood. In short, next/dynamic could be considered a legacy solution.

I can't speak for the Next.js team, but I guess adding { suspense: true } (#27611) is just to help migrate current next/dynamic usage to use the React 18 feature.

cc @huozhi Correct me if I am wrong.

[SukkaW]

Another thought:

When to use next/dynamic with { suspense: false }

• Using React 17 which has no SSR support for React.lazy
  • Maybe people choose React 17 for IE 11 support, who knows?
• Want to benefit from the traditional (blocking) SSR / SSG
• Using React 18 with next export (SSG), so no streaming SSR

When to use React.lazy or next/dynamic with { suspense: true }

• Using React 18
• Want to benefit from React 18 streaming SSR, selective hydration, and more
  • Like experimenting with suspense-based data fetching on the server-side, etc.

[gaearon]

Hmm. I was under the impression that there’s some Babel or webpack plugin that inserts preload hints regardless of the options argument. Maybe that’s wrong.

[huozhi]

next/dynamic with optional suspense: true will opt into React.lazy with react 18, and dynamic components doesn't ollect any dynamic module id for preloading and fully delegate the SSR job to React.lazy.

The issue seems to be that it's hard to tree-shake the shim out of the bundle when the condition is relying on the runtime. We need some extra hint (like process env var for react 18 in next.js) for bundler to erase it properly

var shim = isServerEnvironment ? useSyncExternalStore$1 : useSyncExternalStore;
var useSyncExternalStore$2 = React.useSyncExternalStore !== undefined ? React.useSyncExternalStore : shim;

[SukkaW]

next/dynamic with optional suspense: true will opt into React.lazy with react 18, and dynamic components doesn't ollect any dynamic module id for preloading and fully delegate the SSR job to React.lazy.

Just a question:

Is there any benefit to using next/dynamic with suspense: true compare to using React.lazy directly? (Besides in the future Next.js could add more optimization and features on top of React.lazy to the next/dynamic)

The issue seems to be that it's hard to tree-shake the shim out of the bundle when the condition is relying on the runtime. We need some extra hint (like process env var for react 18 in next.js) for bundler to erase it properly

IMHO the issue is little more than that.

When suspense: true is used, <LazyImpl /> instead of <LoadableImpl /> will be returned. And useSyncExternalStore is only used inside <LoadableImpl />, it is not used in <LazyImpl />, so when suspense: true is used, there will be no useSyncExternalStore usage at all.

And even more than that. In the end, if a website only uses suspense: true (like React Beta Docs!), the main bundle still ships the entire react-loadable client-side implementation.

(But if you use React.lazy directly, you just get away with those react-loadable client-side implementations from your bundle)

[huozhi]

Previously we're trying to add a support for next/dynamic to work with dynamic components in early stage of react 18, the suspense option was added for migration at that time to let you you migrate your next app from react 17 to 18 without using React.lazy explicitly. It wasn't designed to opt-in to React.lazy when react 18 is used 'cause other options don't fit well, such as loading option might require introducing Suspense boundary for fallback usage. Now look back it doesn't have a strong opinion to keep recommending it.

We can recommend the React.lazy for dynamic component loading by default on the next website, that will be easier IMO. And let next/dynamic opt-in React.lazy when using react 18. Later by later, next/dynamic usages will reduce.

[huozhi]

We add chunk preloading support for next/dynamic with suspense: true in v12.1.7-canary.19

[github-actions]

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.