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

chore: retry lazy load components when server is down [WD-13909] #927

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Sep 26, 2024

Done

  • support retry lazy loading modules with increasing delay
  • add more info for dynamic module loading error in the Error page

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Load LXD-UI initially, don't navigate anywhere so that the pages are not cached
    • Open dev tool, go to the Network tab and select Offline as network condition
    • Go to a different page in the UI, it may not be very noticeable but it will now take a little longer for the error page to show because we will now retry fetch the module a few times before throwing an error (this will be more noticeable if you increase the delayTime input for the lazyWithRetry function.
    • See that the error message includes details about lxd server being down if it's a dynamic module loading error

Screenshots

Screenshot from 2024-09-26 10-27-29

@webteam-app
Copy link

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const lazyWithRetry = <T extends React.ComponentType<any>>(
importFunction: () => Promise<{ default: T }>,
retries = 3,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edlerd @Kxiru open to suggestions for reasonable retries and delay time. Note delayTime here essentially is the ceiling of how long we would wait to retry import again (for the final attempt)

src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
src/components/ErrorPage.tsx Outdated Show resolved Hide resolved
src/components/ErrorPage.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to not work as expected. I blocked one of the js files manually with chrome dev tools and see only one request for the resource, where I would expect 3 network requests due to the retry.

@edlerd
Copy link
Collaborator

edlerd commented Sep 26, 2024

image

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 26, 2024

This seems to not work as expected. I blocked one of the js files manually with chrome dev tools and see only one request for the resource, where I would expect 3 network requests due to the retry.

Thanks for catching this! When I tested it, I debugged in the source file and didn't check the network requests! So it seems that the browser caches import responses, which means when we retry, it will use the cache first and hence you are not seeing the additional requests. I'm have an idea, but will need to investigate on the feasibility of the approach, looking into it.

Edit: @edlerd so I pushed an approach, I would like to know what you think about it. Due to React.lazy caching success or failed promises for module import network calls, we need to invalidate the cache when retrying an import. To do this, we can try extract the module url from the error message and add a dynamic query param to force cache invalidation. This unfortunately is a best effort approach, since the error message may or may not contain the module url depending on the browser. However, I think this is acceptable since worst case scenario the error boundary will render. See this related issue regarding React.lazy, there are some approaches suggested which I think all look not so great. TBH, I'm also happy to not introduce this complexity, and maybe we just improve the error page rendered text? wdyt?

@mas-who mas-who force-pushed the better-server-down-error branch 5 times, most recently from 1ae63a8 to 298a5c9 Compare September 26, 2024 17:20
@edlerd
Copy link
Collaborator

edlerd commented Sep 27, 2024

Edit: @edlerd so I pushed an approach, I would like to know what you think about it. Due to React.lazy caching success or failed promises for module import network calls, we need to invalidate the cache when retrying an import. To do this, we can try extract the module url from the error message and add a dynamic query param to force cache invalidation. This unfortunately is a best effort approach, since the error message may or may not contain the module url depending on the browser. However, I think this is acceptable since worst case scenario the error boundary will render. See this related issue regarding React.lazy, there are some approaches suggested which I think all look not so great. TBH, I'm also happy to not introduce this complexity, and maybe we just improve the error page rendered text? wdyt?

If we have a way "auto-heal" I think it is worth the complexity as it is contained within on file. I think there is a bit simpler approach using recursion as described in this blog. Not sure if this one still works, but maybe we can adopt it here?

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 27, 2024

If we have a way "auto-heal" I think it is worth the complexity as it is contained within on file. I think there is a bit simpler approach using recursion as described in this blog. Not sure if this one still works, but maybe we can adopt it here?

Thanks for the resource :) I did see this one and tried it, most of the articles I've seen have similar recursive or iterative implementations. The problem is really that a dynamic import would cache successful or failed network requests i.e. import(<module>) would return a promise to React.lazy, if it failed, then future import calls for the same module, within the same browser session would fail because the cached failure response. Here is a good reference about this issue, also I found the workaround with the query params there. The current approach in this PR works on chrome and firefox, but I'm not sure about safari. afaik, the only reliable way to get around the dynamic import cache issue is to do a window.location.reload, but I think this does not result in a good user experience.

@edlerd
Copy link
Collaborator

edlerd commented Sep 27, 2024

Thanks for the resource :) I did see this one and tried it, most of the articles I've seen have similar recursive or iterative implementations. The problem is really that a dynamic import would cache successful or failed network requests i.e. import(<module>) would return a promise to React.lazy, if it failed, then future import calls for the same module, within the same browser session would fail because the cached failure response. Here is a good reference about this issue, also I found the workaround with the query params there. The current approach in this PR works on chrome and firefox, but I'm not sure about safari. afaik, the only reliable way to get around the dynamic import cache issue is to do a window.location.reload, but I think this does not result in a good user experience.

Yes, reload should be avoided. I suspected the recursive approach might avoid the caching behaviour.

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 27, 2024

Thanks for the resource :) I did see this one and tried it, most of the articles I've seen have similar recursive or iterative implementations. The problem is really that a dynamic import would cache successful or failed network requests i.e. import(<module>) would return a promise to React.lazy, if it failed, then future import calls for the same module, within the same browser session would fail because the cached failure response. Here is a good reference about this issue, also I found the workaround with the query params there. The current approach in this PR works on chrome and firefox, but I'm not sure about safari. afaik, the only reliable way to get around the dynamic import cache issue is to do a window.location.reload, but I think this does not result in a good user experience.

Yes, reload should be avoided. I suspected the recursive approach might avoid the caching behaviour.

Unfortunately not, caching happens within import(), so recursion or iterating the retries does not make a difference. Either than window.location.reload, I think other methods will be "best effort" approaches as this is a dynamic import behaviour that affects all UI frameworks, particularly SPAs I think since poor network connectivity affects chunk loading and there is no reliable way to heal

@mas-who mas-who force-pushed the better-server-down-error branch 2 times, most recently from b1b6394 to 7b686b6 Compare September 30, 2024 17:21
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA LGTM, nice approach :)

Some comments on the code and copy. should be very easy to address and then this should be good to merge.

src/components/ErrorPage.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
src/util/lazyWithRetry.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mas-who mas-who merged commit dd22c99 into canonical:main Oct 1, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants