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.

Sign up for GitHub

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

Jump to bottom

[WIP] Migrate Balance.jsx -> Balance.tsx #2329

Closed
wants to merge 5 commits into from
Closed

[WIP] Migrate Balance.jsx -> Balance.tsx #2329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c4cc5c9
Migrate `Balance.jsx` to Typescript
jfdoming Feb 4, 2024
5d9d9fb
Release notes
jfdoming Feb 4, 2024
efa1eb3
Linter
jfdoming Feb 4, 2024
0a2942d
Strict linter
jfdoming Feb 4, 2024
953be17
Merge branch 'master' into jfdoming/balance-typescript
jfdoming Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 47 additions & 16 deletions ...lient/src/components/accounts/Balance.jsx → ...lient/src/components/accounts/Balance.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import React from 'react';

import { useCachedSchedules } from 'loot-core/src/client/data-hooks/schedules';
import { q } from 'loot-core/src/shared/query';
import { q, type Query } from 'loot-core/src/shared/query';
import { getScheduledAmount } from 'loot-core/src/shared/schedules';
import type { AccountEntity, TransactionEntity } from 'loot-core/types/models';

import { useSelectedItems } from '../../hooks/useSelected';
import { SvgArrowButtonRight1 } from '../../icons/v2';
Expand All @@ -16,7 +17,22 @@ import { useFormat } from '../spreadsheet/useFormat';
import { useSheetValue } from '../spreadsheet/useSheetValue';
import { isPreviewId } from '../transactions/TransactionsTable';

function DetailedBalance({ name, balance, isExactBalance = true }) {
type BalanceQuery = {
name: string;
query: Query;
};

type DetailedBalanceProps = {
name: string;
balance: number;
isExactBalance?: boolean;
};

function DetailedBalance({
name,
balance,
isExactBalance = true,
}: DetailedBalanceProps) {
const format = useFormat();
return (
<Text
Expand All @@ -39,10 +55,15 @@ function DetailedBalance({ name, balance, isExactBalance = true }) {
);
}

function SelectedBalance({ selectedItems, account }) {
type SelectedBalanceProps = {
selectedItems: Set<string>;
account: AccountEntity;
};

function SelectedBalance({ selectedItems, account }: SelectedBalanceProps) {
const name = `selected-balance-${[...selectedItems].join('-')}`;

const rows = useSheetValue({
const rows = useSheetValue<Pick<TransactionEntity, 'id'>[]>({
name,
query: q('transactions')
.filter({
Expand All @@ -54,7 +75,7 @@ function SelectedBalance({ selectedItems, account }) {
const ids = new Set((rows || []).map(r => r.id));

const finalIds = [...selectedItems].filter(id => !ids.has(id));
let balance = useSheetValue({
let balance = useSheetValue<number>({
name: name + '-sum',
query: q('transactions')
.filter({ id: { $oneof: finalIds } })
Expand All @@ -77,6 +98,7 @@ function SelectedBalance({ selectedItems, account }) {
isExactBalance = false;
}

scheduleBalance ??= 0;
if (!account || account.id === s._account) {
scheduleBalance += getScheduledAmount(s._amount);
} else {
Expand Down Expand Up @@ -104,15 +126,17 @@ function SelectedBalance({ selectedItems, account }) {
);
}

function MoreBalances({ balanceQuery }) {
const cleared = useSheetValue({
name: balanceQuery.name + '-cleared',
query: balanceQuery.query.filter({ cleared: true }),
});
const uncleared = useSheetValue({
name: balanceQuery.name + '-uncleared',
query: balanceQuery.query.filter({ cleared: false }),
});
function MoreBalances({ balanceQuery }: { balanceQuery: BalanceQuery }) {
const cleared =
useSheetValue<number>({
name: balanceQuery.name + '-cleared',
query: balanceQuery.query.filter({ cleared: true }),
}) ?? 0;
const uncleared =
useSheetValue<number>({
name: balanceQuery.name + '-uncleared',
query: balanceQuery.query.filter({ cleared: false }),
}) ?? 0;

return (
<View style={{ flexDirection: 'row' }}>
Expand All @@ -122,13 +146,20 @@ function MoreBalances({ balanceQuery }) {
);
}

type BalancesProps = {
balanceQuery: BalanceQuery;
showExtraBalances: boolean;
onToggleExtraBalances: () => void;
account: AccountEntity;
};

export function Balances({
balanceQuery,
showExtraBalances,
onToggleExtraBalances,
account,
}) {
const selectedItems = useSelectedItems();
}: BalancesProps) {
const selectedItems = useSelectedItems<string>();

return (
<View
Expand Down
6 changes: 3 additions & 3 deletions packages/desktop-client/src/components/budget/BalanceWithCarryover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ export function BalanceWithCarryover({
carryoverStyle,
}: BalanceWithCarryoverProps) {
const carryoverValue = useSheetValue(carryover);
const balanceValue = useSheetValue(balance);
const goalValue = useSheetValue(goal);
const budgetedValue = useSheetValue(budgeted);
const balanceValue = useSheetValue<number>(balance);
const goalValue = useSheetValue<number>(goal);
const budgetedValue = useSheetValue<number>(budgeted);
const isGoalTemplatesEnabled = useFeatureFlag('goalTemplatesEnabled');
return (
<View style={style}>
Expand Down
4 changes: 2 additions & 2 deletions packages/desktop-client/src/components/budget/report/budgetsummary/ExpenseProgress.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ type ExpenseProgressProps = {
target: ComponentProps<typeof CellValue>['binding'];
};
export function ExpenseProgress({ current, target }: ExpenseProgressProps) {
let totalSpent = useSheetValue(current) || 0;
const totalBudgeted = useSheetValue(target) || 0;
let totalSpent = useSheetValue<number>(current) || 0;
const totalBudgeted = useSheetValue<number>(target) || 0;

// Reverse total spent, and also set a bottom boundary of 0 (in case
// income goes into an expense category and it's "positive", don't
Expand Down
4 changes: 2 additions & 2 deletions packages/desktop-client/src/components/budget/report/budgetsummary/IncomeProgress.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ type IncomeProgressProps = {
target: ComponentProps<typeof CellValue>['binding'];
};
export function IncomeProgress({ current, target }: IncomeProgressProps) {
let totalIncome = useSheetValue(current) || 0;
const totalBudgeted = useSheetValue(target) || 0;
let totalIncome = useSheetValue<number>(current) || 0;
const totalBudgeted = useSheetValue<number>(target) || 0;

let over = false;

Expand Down
5 changes: 3 additions & 2 deletions packages/desktop-client/src/components/budget/report/budgetsummary/Saved.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ type SavedProps = {
style?: CSSProperties;
};
export function Saved({ projected, style }: SavedProps) {
const budgetedSaved = useSheetValue(reportBudget.totalBudgetedSaved) || 0;
const totalSaved = useSheetValue(reportBudget.totalSaved) || 0;
const budgetedSaved =
useSheetValue<number>(reportBudget.totalBudgetedSaved) || 0;
const totalSaved = useSheetValue<number>(reportBudget.totalSaved) || 0;
const format = useFormat();
const saved = projected ? budgetedSaved : totalSaved;
const isNegative = saved < 0;
Expand Down
6 changes: 4 additions & 2 deletions packages/desktop-client/src/components/budget/rollover/BalanceTooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ export function BalanceTooltip({
onClose,
...tooltipProps
}: BalanceTooltipProps) {
const carryover = useSheetValue(rolloverBudget.catCarryover(categoryId));
const balance = useSheetValue(rolloverBudget.catBalance(categoryId));
const carryover = useSheetValue<boolean>(
rolloverBudget.catCarryover(categoryId),
);
const balance = useSheetValue<number>(rolloverBudget.catBalance(categoryId));
const [menu, setMenu] = useState('menu');

const _onClose = () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/budget/rollover/budgetsummary/ToBudget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function ToBudget({
}: ToBudgetProps) {
const [menuOpen, setMenuOpen] = useState(null);
const sheetName = useSheetName(rolloverBudget.toBudget);
const sheetValue = useSheetValue({
const sheetValue = useSheetValue<string>({
name: rolloverBudget.toBudget,
value: 0,
});
Comment on lines +49 to 52
Copy link
Member

Choose a reason for hiding this comment

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

💭 thought: ‏this is what I was afraid of. We manually type this as a string, but the value here is clearly a number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When sheetValue is used below, it's passed into parseInt, so I assumed it would be a string. Maybe I'm misunderstanding how useSheetValue works?

Copy link
Member

Choose a reason for hiding this comment

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

For now - ignore how sheetValue is used. Focus on the three highlighted lines.

We provide value: 0 in there as the default value. And then we also manually type the return type as string. So which type will be returned? According to the typings: string. But in reality it could as well be number.

Sample: https://www.typescriptlang.org/play?#code/GYVwdgxgLglg9mABCAzgUwMoAs1qgNQEMAbENAHgBUA+ACgDcSyAuZMAazDgHcwBKVpUQBvAFCIJiAE54QUpI1JpEhFIkoBuUQF9RoiAhRREKHHiJLEAXmTpsuAkwpGpMMAHM6ABj5bRAen9EGDUAA1MHCzJQlUQwEABbACM0KUQ4NMITKFcPAH4gA

Expand Down
5 changes: 4 additions & 1 deletion packages/desktop-client/src/components/spreadsheet/useSheetValue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import { useSheetName } from './useSheetName';

import { type Binding } from '.';

export function useSheetValue(binding: Binding, onChange?: (result) => void) {
export function useSheetValue<T>(
binding: Binding,
onChange?: (result) => void,
): T | null {
Comment on lines +10 to +13
Copy link
Member

Choose a reason for hiding this comment

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

❓ question: ‏any way we could make this smarter? For example: I see that binding can have a value. In such case we should be able to extract the type from there. Perhaps we can also extract the type from the binding itself my the name of it?

Basically I'm afraid that manual type definitions here will lead to subtle bugs later.

const { sheetName, fullSheetName } = useSheetName(binding);

const bindingObj =
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ export function SheetCell({
privacyFilter,
} = valueProps;

const sheetValue = useSheetValue(binding, e => {
const sheetValue = useSheetValue<string>(binding, e => {
// "close" the cell if it's editing
if (props.exposed && inputProps && inputProps.onBlur) {
inputProps.onBlur(e);
Expand Down
4 changes: 2 additions & 2 deletions packages/desktop-client/src/hooks/useSelected.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ export function useSelectedDispatch() {
return useContext(SelectedDispatch);
}

export function useSelectedItems() {
return useContext(SelectedItems);
export function useSelectedItems<Item>() {
return useContext(SelectedItems) as Set<Item>;
Copy link
Member

Choose a reason for hiding this comment

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

🔨 warning: ‏manual type coercions lead to subtle bugs that are hard to spot.

Could we instead change line 248 to be

const SelectedItems = createContext<Set<string>>(null);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but I was concerned that the different usages of SelectedItems wouldn't all be strings. I'll double check and fix if they are

}

type SelectedProviderProps<T extends Item> = {
Expand Down
19 changes: 16 additions & 3 deletions packages/loot-core/src/client/data-hooks/schedules.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
// @ts-strict-ignore
import React, { createContext, useEffect, useState, useContext } from 'react';
import React, {
createContext,
useEffect,
useState,
useContext,
type ReactNode,
} from 'react';

import { q, type Query } from '../../shared/query';
import { getStatus, getHasTransactionsQuery } from '../../shared/schedules';
Expand Down Expand Up @@ -66,9 +72,16 @@ export function useSchedules({
return data;
}

const SchedulesContext = createContext(null);
const SchedulesContext = createContext<UseSchedulesReturnType>(null);

export function SchedulesProvider({ transform, children }) {
type SchedulesProviderProps = UseSchedulesArgs & {
children: ReactNode;
};

export function SchedulesProvider({
transform,
children,
}: SchedulesProviderProps) {
const data = useSchedules({ transform });
return (
<SchedulesContext.Provider value={data}>
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/2329.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [jfdoming]
---

Migrate `Balance.jsx` -> `Balance.tsx`
Loading