react icon indicating copy to clipboard operation
react copied to clipboard

Bug: React.lazy + Suspense issue

Open micopiira opened this issue 3 years ago • 6 comments

React version: 18.2.0

Steps To Reproduce

  1. I have a simple "Icon" component that lazily imports other components using React.lazy:
import React from "react";

function Icon({ icon, ...props }) {
  const LazyIcon = React.lazy(() => import(`./icons/${icon}`));
  return <LazyIcon {...props} />;
}

export default Icon;
  1. Then I try to use the said Icon component like this:
<React.Suspense fallback={<p>Loading...</p>}>
  <Icon icon="Foo" />
  <Icon icon="Bar" />
</React.Suspense>
  1. It will never finish loading and only show the fallback

Link to code example:

https://codesandbox.io/s/epic-hamilton-on6e0i

The current behavior

Only shows the fallback "Loading..."

The expected behavior

Render the ./icons/Foo and ./icons/Bar components

micopiira avatar Jul 09 '22 10:07 micopiira

React distinguishes components by referential equality. This means that in your example React always sees a completely different component which needs to load something when rendering. It then suspends and waits for the import to resolve. But every time the component resolves React sees a new component i.e. React will never render the resolved import.

The fix is to ensure that React only sees a new component when the import changes:

-const LazyIcon = React.lazy(() => import(`./icons/${icon}`));
+const LazyIcon = useMemo(() => React.lazy(() => import(`./icons/${icon}`)), [icon]);

eps1lon avatar Jul 09 '22 10:07 eps1lon

@eps1lon Thanks, I tried this:

import React, { useMemo } from "react";

function Icon({ icon, ...props }) {
  const LazyIcon = useMemo(() => React.lazy(() => import(`./icons/${icon}`)), [
    icon
  ]);
  return <LazyIcon {...props} />;
}

export default Icon;

But it still seems to not render, only displaying the fallback. Codesandbox: https://codesandbox.io/s/hardcore-einstein-qmw9o5?file=/src/Icon.js

micopiira avatar Jul 09 '22 10:07 micopiira

Ah yeah because these are siblings and React throws away the results of useMemo if it didn't commit.

eps1lon avatar Jul 09 '22 10:07 eps1lon

I think the pattern is not valid in general. Lazy should always either be hoisted to the top level of your file or cached outside of React.

I understand the author tries to lazily load a particular icon by name. This doesn’t seem like a good idea to me from the performance point of view, or that it would even work as intended. Naïvely this would create a lot of chunks and slow down actually displaying the component. But I guess webpack would optimize that and probably get rid of unnecessary split points. Still I’d suggest to split the higher level units of UI (like dashboard, comments UI, etc) and let the icons be regular imports that get bundled with those chunks depending on which icons are used.

If the author feels strongly about using this pattern, they need to cache the lazy instances in a top-level Map by icon name.

gaearon avatar Jul 09 '22 12:07 gaearon

Duplicate of https://github.com/facebook/react/issues/24534

vkurchatkin avatar Jul 09 '22 14:07 vkurchatkin

@micopiira You can't provide and consume a suspender in one component, but you can do this: https://codesandbox.io/s/dry-feather-u4ourl?file=/src/App.js

heineiuo avatar Jul 12 '22 02:07 heineiuo

experiencing the same issue +1

As a temp compromise, we remove the Suspense for those lazy-loading components. The js bundle will still be lazy loaded properly. We just lose the loading fallback from Suspense.

But agree with what @gaearon mentioned, we should also consider if we want to lazy load the children component

rongdi1993 avatar Feb 09 '23 06:02 rongdi1993