-
Notifications
You must be signed in to change notification settings - Fork 21
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
Infinite Loading on Failed to load data on packages #271
Comments
setIsLocading(false)
in the finally block? If there is an error, the loader will still show, since catch block does not set isLoading to false, is it the behaviour we want?
in all of our packages we are not dealing with situations where loading fails or there is something wrong in API calls to load data. any idea what approach should we take to deal with this ? @KipSigei @p-netm @machariamuguku |
@ashfahan we had the behavior described by @dubdabasoduba here but you removed it. I'd root for that coupled with react error boundary |
Yeah, we should rely on react-error-boundary but only for non-recoverable errors. For API call errors, which I consider recoverable we should only show loading states when the request is pending or some other aysnc operation is in progress. To reiterate, If the there is no pending async operation in process that the user should be exposed to, the view should render successfully or display an error page with some information on what erred and links to other working parts of the application that might be of interest to the user. We should thus only use variables that denote the progress of an async operation in conditions to decide if and how to render the loader. However subject to human review , there might be a few edge cases where this might not be the correct approach |
Pinning this here for easy reference https://tkdodo.eu/blog/status-checks-in-react-query |
Maybe we should have
setIsLocading(false)
in the finally block? If there is an error, the loader will still show, since catch block does not set isLoading to false, is it the behaviour we want?Originally posted by @kelvin-muchiri in #74 (comment)
The text was updated successfully, but these errors were encountered: