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

React.lazy remounts component if used within a render #14299

Closed
danielkcz opened this issue Nov 21, 2018 · 22 comments
Closed

React.lazy remounts component if used within a render #14299

danielkcz opened this issue Nov 21, 2018 · 22 comments

Comments

@danielkcz
Copy link

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

Simple reproduction here: https://codesandbox.io/s/6z4qmm2k7n

The contrived example does not make much sense, but I got burned by this in the context of react-router. I simply wanted to avoid defining all lazily loaded components up front but instead have them coupled with a particular route.

function Routing() {
  <>
    <Route path="foo" component={lazy(() => import('./foo'))} />
  </>
}

What is the expected behavior?

In case this is intended behavior, it should be definitely documented. Otherwise it would be nice to have some memoized lazy variant. Possibly with hooks coming what about useLazy ? :)

@gaearon
Copy link
Collaborator

gaearon commented Nov 21, 2018

This is how regular React works.

function Routing() {
  <>
    <Route path="foo" component={function Foo() { /* ... */ }} />
  </>
}

has exactly the same issues. It's always bad practice.

If you feel concrete suggestions on whether this could be better documented, please feel free to send a doc PR.

@gaearon gaearon closed this as completed Nov 21, 2018
@danielkcz
Copy link
Author

danielkcz commented Nov 21, 2018

Yes, I understand why it's happening, but cannot it be improved? We are heading toward function components that are using memoization and in my opinion, it won't be that uncommon that people would try and possibly want to define resources within a component to improve colocation.

Your example with defining a component within a render can be tackled with useMemo. It's bad practice, no argue there. However, with React.lazy it kinda makes sense to be able to do that. When I tried to memoize result of React.lazy it did not help. I am not entirely sure why, though.

I think it's a slightly different case and it could use some second thought.

@danielkcz
Copy link
Author

danielkcz commented Nov 21, 2018

I did another example based on Alpha and with useMemo just to show that it cannot be handled in user code, most likely because the Suspense is in the play here.

I am not trying to convince you that this is a feature that must be. It's just a suggestion what might be interesting to tackle in the future. I am sure there might be several reasons I just don't see, especially the complexity of Suspense still kinda eludes me.

I will send the docs PR soon-ish.

@theKashey
Copy link
Contributor

I reckon - it never should work. You are giving a new type to React, it should unmount the old one, and mount a new one. This is NOT a remount.

@danielkcz
Copy link
Author

danielkcz commented Dec 30, 2018

But how do you explain it's possible to memoize regular component, but not the lazy one? That's what memoize should do, right? Keep the same object over renders.

@danielkcz
Copy link
Author

danielkcz commented Dec 30, 2018

Ok wow, nevermind, fresh look after some time and I did spot a problem right away...

const LazyLoadedMemo = React.useMemo(() => LazyLoaded);

I am missing the second arg to useMemo, once I add [] to denote no dependencies it starts working properly. That's epic fail :D

Disclaimer: Nonetheless, since useMemo is not guaranteed, it shouldn't be used like this for sure. It can randomly cause remount of the component and possibly causing some loss of user data.

@ashkank83
Copy link

@FredyC Seems as I'm encountering a very similar issue. Did you manage to find a fix? I have created an example here sandbox. Would you please have a look and recommend a solution (if you know one?)

@danielkcz
Copy link
Author

danielkcz commented Jul 4, 2019

@ashkank83 Some dirty solution is the following, but I don't like global variables, so I don't recommend doing that. Also, I can imagine it would be suffering from race conditions. Otherwise, I don't think there a better solution that's officially supported.

const cached = {}

function DynamicLoader(props) {
  if (!cached[props.component]) {
    const LazyComponent = React.lazy(() => import(`${props.component}`))
    cached[props.component] = (
      <Suspense fallback={<div>Loading...</div>}>
        <LazyComponent />
      </Suspense>
    )
  }
  return cached[props.component]
}

@ashkank83
Copy link

ashkank83 commented Jul 4, 2019

@FredyC Thanks for the reply. Since posting the question, I got the following as well as a potential solution

const DynamicLoader = React.memo(function(props: any) {
  const LazyComponent = React.lazy(() => import(`${props.component}`));
  return (
    <Suspense fallback={<div>Loading...</div>}>
      <LazyComponent />
    </Suspense>
  );
});

Not sure if there are cons and pro to the different solutions. As you said as well, I prefer to avoid the global variable, so maybe React.memo is prefered. What do you think?

@danielkcz
Copy link
Author

danielkcz commented Jul 4, 2019

@ashkank83 That's probably better, but it depends how permanent is a cache of React.memo. We know that useMemo is not reliable and can be recomputed randomly. If a similar thing applies to React.memo you might randomly lose state. Docs state the following...

This method only exists as a performance optimization. Do not rely on it to “prevent” a render, as this can lead to bugs.

Sounds risky enough to me. I don't know, I've given up on that as it's not that horrible to do it recommended with manual calls of React.lazy.

@dhedey
Copy link

dhedey commented Jul 5, 2019

For the benefit of others reading this, an approach to persisting the lazy references useRef is in my comment here: reactjs/react.dev#1422 (comment)

As FredyC refers to in previous comments, this has benefits over useMemo (which isn't guaranteed to be persist, risking the reference and hence state changing).

Using global module state (as in Fredy's above comment) is possibly preferable to useRef, depending on whether you want to lost the reference / get a new reference when the parent element gets unmounted/remounted.

@jvivasclimate
Copy link

@gaearon any insights on this? I considered React.lazy to have the same behavior as a regular react component. Meaning that it will be "rendered" equally as other Components. But the Component instance changes after every render.
@FredyC we are planning on using something pretty similar to what you suggested. Not sure if its the best idea to hold a dictionary of Components just to keep the instance alive.

@theKashey
Copy link
Contributor

Again - this is how it should work.
Lazy creates a complex object, which tracks the promise resolution state inside it and that state should be initiated after component creation (as long as Lazy is a pure function).
That basically means - lazy would not be "ready" for use just after creation, except the cases with synchronous thenables dynamic imports are not.

@Haraldson
Copy link

I’m experiencing a similar issue with react-router, React.lazy and React.Suspense. For now it’s making lazily loaded top-level routes unusable, in cases where there are nested routes as well. In such a scenario, my top-level route’s component will also act as sort of a layout for the nested stuff, but because of the top-level React.Suspense, I will get a flashing loading state from it no matter what. What I want is, of course, for all the static layout elements within the top-level route’s component to stay put.

This in turn comes from how react-router works: My top-level <Route> can’t be set to be exact because its component need to stay in place to handle its sub-routes. Thus, when you follow links to nested paths, both the top-level and the sub-level route will match anew, and their components will be remounted.

Now, if only React.lazy would somehow keep track of which bundles had already been lazily imported, and return those instead of fetching them again, we wouldn’t have to go through this whole dance, because React.Suspense wouldn’t be notified since no async stuff would happen.

@theKashey
Copy link
Contributor

Now, if only React.lazy would somehow keep track of which bundles had already been lazily imported

Of course it never will do it - there is no way to know what is "behind import" before calling it. Use "SSR-friendly" code splitting libraries to "track" "loaded" "bundles"

@Haraldson
Copy link

Haraldson commented Nov 18, 2019

So what I’m reading between the snark here is that React.lazy, with its default to always refetch code, isn’t really for anyone but a fast-paced ‘Facebook’ where reality dictates that code indeed could have changed, even between two calls to load the same bundle within the same session?

I’m not concerned with SSR for the moment, why are you recommending to use SSR techniques to solve a problem you said isn’t a problem and isn’t solvable in the sentence before?

@theKashey
Copy link
Contributor

Because you are missing the point @Haraldson.

  • React.lazy is always returning a new component. Because it's a factory
  • every lazy has an internal state, which knows is it not-loader/loading or loaded. Once it "loaded" - it never refetches the code. NEVER.
  • lazy expects some importer - a function which will return a Promise with default, and it could be anything. It could not, and should "think" what it is, and every time you will give it a "new" importer. The same problem as with "new" lazy itself.
  • However, SSR requires to "knew" which bundles has to be loaded to fulfil requirements for "lazy components" on the current page. This is "the track" you are looking for.

For example - loadable-components will use babel-plugin to extract file name from import, and will synchronously import required component, as long as they "knew" - their code is loaded.
However, as long as any factory will generate a unique result every time - you will experience a full tree remount. Like using a different key on the node.

So, please, just try to understand how it works, why it works that way, and why it's not working for your case, and what's is wrong with your case.

@maulik-soni
Copy link

Implementation:

const LazyloadComponent = componentPath => (showLoader = true) => {
  return props => {
    let [ComponentContainer, setComponentContainer] = useState(null);

    useEffect(() => {
      const path = componentPath();
      setComponentContainer(lazy(() => path));
    }, []);

    return (
      <Suspense fallback={showLoader ? <LoadingSpinner /> : null}>
        {ComponentContainer && <ComponentContainer {...props} />}
      </Suspense>
    );
  };
};

Usage :

const Component = LazyloadComponent(() =>
  import('../components/your/path')
)(true || false); // flag for hide/show of loader in suspense.

@hypeJunction
Copy link

Solution from @maulik-soni works for me if I wrap the output in useMemo

const CodesIndex = useMemo(() => LazyloadComponent(() => import('./Screens/Codes'))(true), []);

return (
   <Route path="/codes" components={CodesIndex} />
);

@landisdesign
Copy link

landisdesign commented Jan 25, 2023

Might I suggest another wrapper option, building on @maulik-soni's work? Use a closure to retain the reference to the to-be-built component:

const buildLazyComponent = (load, fallback = null) => {
  let LoadedComponent = null;

  const LazyComponent = forwardRef(
    (props, ref) => {
      if (!LoadedComponent) {
        LoadedComponent = lazy(load);
      }
      return (
        <Suspense fallback={fallback}>
          <LoadedComponent {...props} ref={ref} />
        </Suspense>
      )
    }
  );

  return LazyComponent;
}

Usage:

const MyComponent = buildLazyComponent(
  () => import('./MyComponent.lazy.jsx'), // or whatever convention lets you distinguish
  <Loading />
);

export default MyComponent;

Edited to include forwardRef based on @ahmed-zhran's catch in the next comment.

@ahmed-zhran
Copy link

ahmed-zhran commented Apr 29, 2023

lazy with forwardRef and memo 🙃:

great wrapper I was trying to use lazy component with forwardedRef component at the same time from a while @landisdesign
...just I wrapped the return function by forwardRef and it works perfectly

import React, { forwardRef, lazy, Suspense } from 'react';

const loadable = (importFunc, { fallback = null } = { fallback: null }) => {
  let LazyComponent = null;
  const loadingComp = forwardRef((props, ref) => {
    !LazyComponent && (LazyComponent = lazy(importFunc));
    return (
      <Suspense fallback={fallback}>
        <LazyComponent {...props} ref={ref} />
      </Suspense>
    )
  });
  return loadingComp;
};


export default loadable;

usage:

export default loadable(() => import('./someComponent.jsx'), <Loading />);

just a note the someComponent should be wrapped on forwardedRef as well if you wish to use the ref and also it can be wrapped with memo with no problems 😊😊😊😊

component example '.someComponent.jsx' :

import React, { memo } from 'react';
import Typography from '@mui/material/Typography';

const SxText = forwardRef(function SxText({children, props}, ref) {
  return (
      <Typography {...props} ref={ref}>
        {children}
      </Typography>
  );
})

SxText.propTypes = {};

export default memo(SxText);

@landisdesign
Copy link

@ahmed-zhran oops! I forgot that part! Good catch. I'll edit my attempt above to put it all in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests