-
Notifications
You must be signed in to change notification settings - Fork 0
add fallback behaviour for is-loading #20
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR makes the isLoading prop optional in the AsyncView component and adds automatic loading state inference based on data and error availability.
Key Changes:
- Made
isLoadingprop optional in the component's type definition - Implemented fallback logic to automatically determine loading state when
isLoadingis undefined - Refactored condition checks to use extracted boolean variables for improved readability
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| import React, { ReactElement, ReactNode } from 'react' | |||
| import { ReactElement, ReactNode } from 'react' | |||
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The React import was removed but ReactElement and ReactNode types are still being imported. Since this component returns JSX (line 43, 47), React needs to be in scope for older React versions or the JSX transform configuration should be verified to ensure the automatic JSX runtime is enabled.
| import { ReactElement, ReactNode } from 'react' | |
| import React, { ReactElement, ReactNode } from 'react' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ok, I guess. It was automatically removed.
| const isError = error !== null && error !== undefined | ||
| const hasData = data !== null && data !== undefined | ||
|
|
||
| if (isLoading || (isLoading === undefined && !hasData && !isError)) { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback loading state logic !hasData && !isError could incorrectly treat the initial state (no data, no error) as loading even after a request completes with empty/falsy data. Consider whether empty arrays, empty strings, or false as valid data values should trigger loading state.
| if (isLoading || (isLoading === undefined && !hasData && !isError)) { | |
| // Show loading only if isLoading is true, or if isLoading is undefined and both data and error are undefined/null (initial state) | |
| if (isLoading === true || (isLoading === undefined && !hasData && !isError)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not true, as we check explicitly for null and undefined.
SirCotare
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! 💯
No description provided.