Correct the AccountOverview.OverviewsResponse TS interface account type to show it is nullable
#7921
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +14 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Because #7920 stemmed from #7883 (comment), I checked to make sure if I removed this line, TS will correctly give a warning that account might be null here. It does, so that's good!
I hope this requires minimal effort to review!
Not too minimal because out of curiosity, I searched all selectors.ts and selectors.js to make sure there isn't other similar case.
Found 1 possible undefined return value in client/data/capital/selectors.ts but it's not a problem there. When I tried to remove this check, TS will throw an error correctly, so I started to think maybe if client/data/deposits/selectors.js is converted to TS, the possible null account will be caught.
Not suggesting to do that though.
This PR looks good as is!
| @@ -115,7 +115,7 @@ const AccountBalances: React.FC = () => { | |||
| currencyCode: overview.currency, | |||
| availableFunds: overview.available?.amount ?? 0, | |||
| pendingFunds: overview.pending?.amount ?? 0, | |||
| delayDays: account.deposits_schedule.delay_days, | |||
| delayDays: account?.deposits_schedule.delay_days, | |||
There was a problem hiding this comment.
tl;dr; this comment is just an FYI. Not suggesting any change.
Tempted to suggest a default value so in accordance of the loading state.
| delayDays: account?.deposits_schedule.delay_days, | |
| delayDays: account?.deposits_schedule.delay_days ?? 0, |
However, in case of account is null (which by itself is impossible to pass the if ( isLoading ) check) account?.deposits_schedule.delay_days being undefined looks more correct:
than if we set delayDays to be 0:
Also I wondered why TS does not complain that delayDays can be undefined although delayDays' type is defined as a number. Maybe because here we don't strictly define the type of depositCurrencyTabs to be BalanceTabProps[].
There was a problem hiding this comment.
Another thing we could do to tidy this up – we can define and return 2 distinct types: one while loading and one loaded. E.g.
export interface OverviewsLoading {
account: null;
isLoading: true;
}
export interface Overviews {
account: Account;
isLoading: false;
}
export type OverviewsResponse = OverviewsLoading | Overviews;
export function isOverviewsLoading(response: OverviewsResponse): response is OverviewsLoading {
return response.isLoading;
}Then for the isLoading check, we can use the type assertion function isOverviewsLoading. If that returns false, all following logic will be confident that account: Account, no need for ?? to account for possible account: null.
There was a problem hiding this comment.
I've used your solution in eaa6dce as a good solution for now 👍
There was a problem hiding this comment.
we can define and return 2 distinct types: one while loading and one loaded
Ow wow, that's actually neat.
💯 if the `/client/data dir was migrated to TS, this issue would have been made apparent before commit. Alternatively, we can migrate existing data fetching and caching logic to react-query (demo in #7502). We'd still have to define the API request return types and tie these to |
Co-authored-by: Shendy <[email protected]>
Jinksi
deleted the
fix/7920-correct-account-overview-account-type-nullable
branch
Fixes #7920
Note
This is a small, dev-facing change. I hope this requires minimal effort to review!
Changes proposed in this Pull Request
This PR fixes a mismatch between the
OverviewsResponseTS interface and data returned fromgetAllDepositsOverviews()while the fetch request is in progress:woocommerce-payments/client/data/deposits/selectors.js
Lines 58 to 64 in b37d805
OverviewsResponsehas been corrected to define theoverviews.accounttype asAccount | null:Testing instructions
npx tsc --noEmitand ensure no TS errors are returned.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge