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 does not allow retrying a rejected promise #14254

Closed
tnunes opened this issue Nov 16, 2018 · 17 comments
Closed

React.lazy does not allow retrying a rejected promise #14254

tnunes opened this issue Nov 16, 2018 · 17 comments
Labels

Comments

@tnunes
Copy link
Contributor

tnunes commented Nov 16, 2018

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

It can be seen as a feature or a bug, depending on angle. Let's say it's an enhancement to how lazy works.

What is the current behavior?

When using React.lazy, if the given promise rejects while trying to asynchronously load a component, it's no longer possible to retry loading the component chunk because lazy internally caches the promise rejection.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

This does not seem to work great in CodeSandbox because it's using service workers, which get in the way when simulating offline mode, yet this small app illustrates the issue: https://codesandbox.io/s/v8921j642l

What is the expected behavior?

A promise rejection should not be cached by lazy and another attempt to render the component should call the function again, giving it the chance to return a new promise.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

AFAIK all version of React that include lazy.

@threepointone
Copy link
Contributor

threepointone commented Nov 26, 2018

React.lazy accepts any async function that returns a module, so you could do something like this -

const Something = React.lazy(async () => {
  let mod;
  while(!mod){ 
    try{
      mod = await import('./something')
    }
    catch(err){}   
  }
  // this does infinite retries, 
  // you could modify it to do it only thrice, 
  // or add backoff/network detection logic, etc 
  return mod 
})

(I haven't tested the above, but I think it should work?)

@tnunes
Copy link
Contributor Author

tnunes commented Nov 26, 2018

@threepointone that would immediately retry loading a failed module until it eventually fails and gives up, which could address part of the problem.

The part we can't currently address unless we change lazy implementation is the scenario where we want to retry loading a previously failed async module load after the initial attempt(s) gave up.

Imagine an app code-split by route:

  1. The user loads the initial route, navigates around while loading new routes on demand and then eventually goes offline, or has connectivity issues.
  2. Any new route requested during this period will fail to load (which is expected) and we want to be able to gracefully show an error letting the user know we couldn't load it right now.
  3. The user is free to continue using the application and navigate back to the routes that have already been loaded.
  4. At a later time, if the user goes back to the previously failed route, we want to retry loading the module from the network and render it if successful.

Since lazy is caching a failed promise, we can't do the 2nd part of 2 in a timely fashion or do 4 at all right now. Changing lazy to cache only fulfilled promises and forget rejected ones would allow us to support this use case, which seems something we would like React to facilitate.

This is applicable to any lazily loaded component, not only at "route-based" split points.

@leoasis
Copy link

leoasis commented Nov 26, 2018

FWIW, I ran into this same problem, and found this issue while doing research as to who was caching the promise (thanks for filing it!). I found a workaround if you still have to make it work until this is properly solved here.

Codesandbox is here: https://codesandbox.io/s/1rvm45z3j3

Basically I created this function, which creates a new React.lazy component whenever the import promise rejects, and assigns it back to the component variable:

let Other;

function retryableLazy(lazyImport, setComponent) {
  setComponent(
    React.lazy(() =>
      lazyImport().catch(err => {
        retryableLazy(lazyImport, setComponent);
        throw err;
      })
    )
  );
}

retryableLazy(
  () => import("./Other"),
  comp => {
    Other = comp;
  }
);

Another important aspect is that the error boundary of your component should be using something like render props, in order to use the new value that the variable references at any point in time (otherwise it will always use the first value assigned to Other and keep using it forever):

<ErrorBoundary>
  {() => (
    <Suspense fallback={<div>Loading...</div>}>
      <Other />
    </Suspense>
  )}
</ErrorBoundary>

Hope this helps at least make it work while this is solved!

@tnunes
Copy link
Contributor Author

tnunes commented Nov 26, 2018

Thanks for sharing @leoasis.

There are workarounds for this in user-land, albeit rather convoluted. Changing lazy can make this common scenario very simple.

Is there any known drawback to this proposed change, like for example affecting performance in a negative way?

@threepointone
Copy link
Contributor

We definitely want to support this in a first class manner from react, but I don’t have any specific details to share yet.
The proposed workarounds aren’t ideal, but if they unblock you and you’re ok with the perf after measuring it, might be ok for now. Will report back on this issue when we have more news.

@hyzhak
Copy link

hyzhak commented Jan 20, 2019

@threepointone is there any updates about this issue?

@gaearon
Copy link
Collaborator

gaearon commented Jan 21, 2019

If there was an update — it would be on this issue. :-)

You can help drive it by submitting a failing test case, with or without a fix. Here's an example of me changing something in React.lazy a few days ago, might help: #14626.

@ramsunvtech
Copy link

Any update on this Issue ???
Rejected and cached the failed result. next time its not triggering the fresh call to retrieve the data

@varoot
Copy link

varoot commented Apr 2, 2019

I have made a pull request #15296 and added a test case. Please review and comment.
Thanks.

@mgtitimoli
Copy link

This could be done at the promise level retrying with a catch handler provided to the promise returned by the call to import, and you can even create an importWithRetries utility where you can pass the number of retries and the path of the file.

@Xesenix
Copy link

Xesenix commented Oct 6, 2019

Handling lazy loading was so much easier with use of react-loadable (unfortunately it doesn't look like its maintained) it shame that its done so poorly in main library.
Also it looks like everybody tries to do automatic retries and what about showing user some message and for example retrying on user action like clicking retry button?

Edit:
I have found a way how to handle errors with retrying on user interaction if anyone needs that:

import * as React from 'react';

export default function LazyLoaderFactory<T = any>(
  promise: () => Promise<{ default: React.ComponentType<any> }>,
  Loader = () => <>Loading...</>,
  ErrorRetryView = ({ retry }) => <button onClick={retry}>Module loading error. Try again.</button>,
) {
  function LazyLoader(props: T) {
    const [ loading, setLoading ] = React.useState<boolean>(true);
    const retry = React.useCallback(() => setLoading(true), []);
    const Lazy = React.useMemo(() => React.lazy(() => promise().catch(() => {
      setLoading(false);
      return { default: () => <ErrorRetryView retry={retry}/> };
    })), [promise, loading]);
    return <React.Suspense fallback={<Loader/>}><Lazy {...props}/></React.Suspense>;
  }

  (LazyLoader as any).displayName = `LazyLoader`;

  return LazyLoader as React.ComponentType<T>;
}

@stale
Copy link

stale bot commented Jan 9, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 9, 2020
@stale
Copy link

stale bot commented Jan 16, 2020

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

@stale stale bot closed this as completed Jan 16, 2020
@Misiu
Copy link

Misiu commented Feb 3, 2020

This issue (feature request) is still valid. I think that retrying should have first-class support.

@briandipalma
Copy link

Yes, this issue isn't fixed. What's wrong with different caching behaviour based on whether the Promise fulfilled or rejected?

@briandipalma
Copy link

For anyone else coming across this issue; I've gone for a slightly different workaround:

<ErrorBoundary>
  {() => (
    <Suspense fallback={<div>Loading component...</div>}>
      {React.createElement(
        React.lazy(() => import("./my/Component")),
        {  aProp: "aValue"  }
      )}
    </Suspense>
  )}
</ErrorBoundary>;

This is re-rendered by a retry button in the ErrorBoundary that changes the ErrorBoundary state.

@juandjara
Copy link

Sorry for bringing back an old post, but I think I've found a way to reproduce this. In my casse, this error only happens when redeploying. In my case i'm using Vite to bundle a React SPA with React Router and a global app-level error boundary.

Say you have a user browsing your deployed app, and you deploy a new version that changes something in a component that is being dynamically loaded. Well, when the deploy finishes, your old version of the dynamically loaded modules would no longer be available because of cache busting, but the new ones will be.

So when the user, that had the tab open with the same url all this time goes to browse another route, the module loading system will crash.

I implemented a very very hacky workaround for this like this (TS Code):

const MODULE_IMPORT_ERROR = 'Failed to fetch dynamically imported module'

function handleError(error: Error) {
  // eslint-disable-next-line
  console.error(error)
  // if the error starts with this message, reload the whole app
  if (error.message.indexOf(MODULE_IMPORT_ERROR) === 0) {
    window.location.reload()
  }
}

export default function ErrorCatcher({ children }: React.PropsWithChildren<unknown>) {
  return (
    <ErrorBoundary FallbackComponent={ErrorMessage} onError={handleError}>
      {children}
    </ErrorBoundary>
  )
}

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

No branches or pull requests