Implement retry in case of error

[ValentinH]

🚀 Feature Proposal

Implement a new "retry" option in @loadable/component.

Motivation

We monitor our errors with Sentry and we see quite a lot of errors coming with lazy-loaded components. Indeed, on flaky network, it's easy to have a request aborted and the component is never loaded. In our applications, we have started to wrap all dynamic imports passed to loadable with a retry function such as:

const Example = loadable(() =>
  retry(
    () => import('./Example'),
    { retries: 3 }
  )
)

where retry is a simple function:

type Options = {
  retries?: number
  interval?: number
  exponentialBackoff?: boolean
}

// taken from https://dev.to/goenning/how-to-retry-when-react-lazy-fails-mb5
export function retry<R>(
  fn: () => Promise<R>,
  { retries = 3, interval = 500, exponentialBackoff = true }: Options = {}
) {
  return new Promise<R>((resolve, reject) => {
    fn()
      .then(resolve)
      .catch((error) => {
        setTimeout(() => {
          if (retries === 1) {
            reject(error)
            return
          }

          // Passing on "reject" is the important part
          retry(fn, {
            retries: retries - 1,
            interval: exponentialBackoff ? interval * 2 : interval,
          }).then(resolve, reject)
        }, interval)
      })
  })
}

Example

const Example = loadable(() => import('./Example'),  {retries: 3})

Pitch

I'm wondering if this is something that could be directly implemented as part of loadable.

[theKashey]

Good point. As a matter of fact - our own Sentry is filled with the same reports.

[ValentinH]

If you agree, I'd be happy to work a PR.

[theKashey]

That could be a little complicated as right now errors are just thrown, expected to be handled on the nearest ErrorBoundary. The question - should network problems be still reported (so the error has to be thrown), or in case they are retry-able loadable can swallow them?

[ValentinH]

I don't know if you are able to differentiate network errors from other kinds of errors when using dynamic imports. Do you know?

Regarding errors just being thrown at the moment, if the retry is optional, we could still retry all errors as it would not be a breaking change. What do you think?

[theKashey]

I don't know if you are able to differentiate network errors

You can, but that is abstraction leaking, so you should not try to differentiate.

Regarding errors just being thrown at the moment...

This moment can alter the implementation a lot. Providing ErrorBoundary with built-in retry mechanism and built-in reporting can be a better solution, and actually can be done right now. Well, almost - such error boundary cannot distinguish LoadableError from any error coming from within loaded content. So we are circling back to the problem of "how to differentiate"

[ValentinH]

I might be missing something but I think the retry function I shared in my issue is only retrying errors related to the dynamic import, not errors within the loaded component.

By the way, I deployed it on our main app in production and almost all the ChunkLoadError are gone.

[theKashey]

If only error boundary could detect chunk loading error (thrown by loadable) - then it can do retry.

Wrapping import with any extra code is breaking SSR. There is an update to a babel plugin to make that impossible as well.

[ValentinH]

Wrapping import with any extra code is breaking SSR. There is an update to a babel plugin to make that impossible as well.

What do you mean by "breaking SSR"? We use this technique on an SSR application and I didn't notice any issues. 🤔

[sosnomIntent]

Hello! Facing the same issue from some time. I would like to try @ValentinH solution, will let you know when it will be tested it on production in my project

[stale[bot]]

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 contributions.

[ValentinH]

I think this issue is still relevant. My last question was not answered actually.

[theKashey]

What do you mean by "breaking SSR"? We use this technique on an SSR application and I didn't notice any issues. 🤔

Loadable babel plugin is reading your imports and create another structure which is expected to "do the same". But logic there is very simply - it reads a file from the import and rewrites it to require for SSR. In other words - all your "additions" will be just discarded.

See test - https://github.com/gregberge/loadable-components/blob/main/packages/babel-plugin/src/snapshots/index.test.js.snap#L27-L56

[ValentinH]

Actually, I'm fine if my addition are discarded for the server bundle. The important thing here is to retry on the browser one. Or do you mean that it will actually break the require on the server?

I checked and on our applications we actually only use loadable in components that are wrapped with NoSsr wrappers.

[theKashey]

It's more about the general unpredictibility of the result. Let's double-check if #511 can solve your issue. Probably with the minimal changes we can teach guard to retry import if needed. cc @hedgepigdaniel

[hedgepigdaniel]

It's more about the general unpredictibility of the result. Let's double-check if #511 can solve your issue. Probably with the minimal changes we can teach guard to retry import if needed. cc @hedgepigdaniel

Hmm maybe - perhaps if guard received a createImportPromise/tryImport (that could be called multiple times to retry) rather than an already created importPromise

[theKashey]

Sounds correct. However, before doing anything we should double-check our ability to measure and report such problems.

So, @ValentinH - how did you found that you have this problem? NewRelic, Sentry, other custom error monitoring? Is there something we can do better here?

[ValentinH]

I found this issue thanks to our Sentry integration. If I recall correctly the error was "Chunkload error". When we started wrapping all our dynamic imports with the retry function I shared in the original post, we noticed a clear decrease of that error.

[Etto91]

hi guys..any update?

[theKashey]

@hedgepigdaniel is busy right now a little but I hope quite soon we'll break the status quo.

[stale[bot]]

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 contributions.

[ValentinH]

This issue is still relevant.

[stale[bot]]

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 contributions.

[ValentinH]

This issue is still relevant.

[kamalbennani]

Any update on this topic, I think it's still relevant what @ValentinH suggested!

[YPCrumble]

I'm currently trying to use the retry function created by @ValentinH to wrap my import statements as follows:

const SomeComponent = loadable(() => retry(() => import('./SomeComponent')))

The code works great (thank you!) but unfortunately it masks the types of the props of SomeComponent. I've tried to import types from loadable__types but haven't found a way to pass the props from retry to loadable.

For instance, this attempt:

import { DefaultComponent } from 'loadable__component'

type RetryType = () => Promise<{ default: React.ComponentType<any> }>

export function retry(
  fn: () => Promise<DefaultComponent<RetryType>>,
  { retries = 3, interval = 500, exponentialBackoff = true }: Options = {}
) {
  return new Promise<<DefaultComponent<RetryType>>((resolve, reject) => {
    fn()
      .then(resolve)
      .catch((error) => {
        setTimeout(() => {
          if (retries === 1) {
            reject(error)
            return
          }

          // Passing on "reject" is the important part
          retry(fn, {
            retries: retries - 1,
            interval: exponentialBackoff ? interval * 2 : interval,
          }).then(resolve, reject)
        }, interval)
      })
  })
}

...gives this error:

...Type 'Promise<typeof import("path/to/file")>' is not assignable to type 'Promise<DefaultComponent<RetryType>>'.

Any suggestions on improving the type of the retry function?

[ValentinH]

@YPCrumble you need to use a generic signature. I have updated my original message with what you are looking for 😉

[YPCrumble]

@ValentinH worked perfectly, thank you @ValentinH !!!

[divyanshu013]

This issue is still problematic, if it's not prioritized / fixable at the moment can we link this in the docs? Discoverability of this solution can help future users from wasting few hours hopefully.

[stale[bot]]

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 contributions.

[stale[bot]]

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 contributions.