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

[TypeScript] Add types to amount utils #4207

Merged
merged 2 commits into from
Jan 21, 2025
Merged

[TypeScript] Add types to amount utils #4207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dbb33f
Add types to amount utils
joel-jeremy Jan 20, 2025
501e3db
Release notes
joel-jeremy Jan 20, 2025
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
10 changes: 5 additions & 5 deletions packages/desktop-client/src/components/modals/ImportTransactionsModal/Transaction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,12 +218,12 @@ export function Transaction({
: {}),
}}
title={
inflow === null && outflow === null
outflow === null
? 'Invalid: unable to parse the value'
: amountToCurrency(outflow)
}
>
{amountToCurrency(outflow)}
{amountToCurrency(outflow || 0)}
</Field>
<Field
width={90}
Expand All @@ -235,12 +235,12 @@ export function Transaction({
: {}),
}}
title={
inflow === null && outflow === null
inflow === null
? 'Invalid: unable to parse the value'
: amountToCurrency(inflow)
}
>
{amountToCurrency(inflow)}
{amountToCurrency(inflow || 0)}
</Field>
</>
) : (
Expand Down Expand Up @@ -271,7 +271,7 @@ export function Transaction({
: amountToCurrency(amount)
}
>
{amountToCurrency(amount)}
{amountToCurrency(amount || 0)}
</Field>
</>
)}
Expand Down
14 changes: 8 additions & 6 deletions packages/desktop-client/src/components/reports/graphs/AreaGraph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ import { renderCustomLabel } from './renderCustomLabel';
type PayloadItem = {
payload: {
date: string;
totalAssets: number | string;
totalDebts: number | string;
netAssets: number | string;
netDebts: number | string;
totalTotals: number | string;
totalAssets: number;
totalDebts: number;
netAssets: number;
netDebts: number;
totalTotals: number;
};
};

Expand Down Expand Up @@ -141,7 +141,9 @@ const customLabel = ({
((typeof props.value === 'number' ? props.value : 0) > 0 ? 10 : -10);
const textAnchor = props.index === 0 ? 'left' : 'middle';
const display =
props.value !== 0 ? `${amountToCurrencyNoDecimal(props.value)}` : '';
typeof props.value !== 'string' && props.value !== 0
? `${amountToCurrencyNoDecimal(props.value || 0)}`
: '';
const textSize = adjustTextSize({ sized: width, type: 'area' });

return renderCustomLabel(calcX, calcY, textAnchor, display, textSize);
Expand Down
14 changes: 7 additions & 7 deletions packages/desktop-client/src/components/reports/graphs/BarGraph.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ type PayloadChild = {
type PayloadItem = {
payload: {
name: string;
totalAssets: number | string;
totalDebts: number | string;
netAssets: number | string;
netDebts: number | string;
totalTotals: number | string;
networth: number | string;
totalChange: number | string;
totalAssets: number;
totalDebts: number;
netAssets: number;
netDebts: number;
totalTotals: number;
networth: number;
totalChange: number;
children: [PayloadChild];
};
};
Expand Down
2 changes: 1 addition & 1 deletion packages/desktop-client/src/components/reports/reports/SpendingCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ export function SpendingCard({
<PrivacyFilter activationFilters={[!isCardHovered]}>
{data &&
(difference && difference > 0 ? '+' : '') +
amountToCurrency(difference)}
amountToCurrency(difference || 0)}
</PrivacyFilter>
</Block>
</View>
Expand Down
55 changes: 37 additions & 18 deletions packages/loot-core/src/shared/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,21 @@ export function getNumberFormat({

// Number utilities

/**
* The exact amount.
*/
export type Amount = number;
Copy link
Member

Choose a reason for hiding this comment

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

Curious about your thoughts on making these branded types so they are not interchangeable? Otherwise correct me if I'm wrong but I think TypeScript would allow substituting IntegerAmount for Amount

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 tried implementing it briefly and it results in a lot of typecheck errors:
image

I think it's a good idea and we should implement it, but we do it in a separate PR. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah sounds good

/**
* The exact amount that is formatted based on the configured number format.
* For example, 123.45 would be '123.45' or '123,45'.
*/
export type CurrencyAmount = string;
/**
* The amount with the decimal point removed.
* For example, 123.45 would be 12345.
*/
export type IntegerAmount = number;

// We dont use `Number.MAX_SAFE_NUMBER` and such here because those
// numbers are so large that it's not safe to convert them to floats
// (i.e. N / 100). For example, `9007199254740987 / 100 ===
Expand All @@ -353,65 +368,69 @@ export function safeNumber(value: number) {
return value;
}

export function toRelaxedNumber(value: string) {
return integerToAmount(currencyToInteger(value) || 0);
export function toRelaxedNumber(currencyAmount: CurrencyAmount): Amount {
return integerToAmount(currencyToInteger(currencyAmount) || 0);
}

export function integerToCurrency(
n: number,
integerAmount: IntegerAmount,
formatter = getNumberFormat().formatter,
) {
return formatter.format(safeNumber(n) / 100);
return formatter.format(safeNumber(integerAmount) / 100);
}

export function amountToCurrency(n) {
return getNumberFormat().formatter.format(n);
export function amountToCurrency(amount: Amount): CurrencyAmount {
return getNumberFormat().formatter.format(amount);
}

export function amountToCurrencyNoDecimal(n) {
export function amountToCurrencyNoDecimal(amount: Amount): CurrencyAmount {
return getNumberFormat({
...numberFormatConfig,
hideFraction: true,
}).formatter.format(n);
}).formatter.format(amount);
}

export function currencyToAmount(str: string) {
export function currencyToAmount(
currencyAmount: CurrencyAmount,
): Amount | null {
let amount;
if (getNumberFormat().separatorRegex) {
amount = parseFloat(
str
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separatorRegex, '.'),
);
} else {
amount = parseFloat(
str
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separator, '.'),
);
}
return isNaN(amount) ? null : amount;
Comment on lines +393 to 410
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve error handling in currencyToAmount.

The function could benefit from more robust error handling:

  1. Consider throwing an error for invalid formats instead of returning null
  2. Add input validation for empty strings
  3. Document the expected format in JSDoc
 export function currencyToAmount(
   currencyAmount: CurrencyAmount,
 ): Amount | null {
+  if (!currencyAmount?.trim()) {
+    return null;
+  }
   let amount;
   if (getNumberFormat().separatorRegex) {
     amount = parseFloat(
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function currencyToAmount(
currencyAmount: CurrencyAmount,
): Amount | null {
let amount;
if (getNumberFormat().separatorRegex) {
amount = parseFloat(
str
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separatorRegex, '.'),
);
} else {
amount = parseFloat(
str
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separator, '.'),
);
}
return isNaN(amount) ? null : amount;
export function currencyToAmount(
currencyAmount: CurrencyAmount,
): Amount | null {
if (!currencyAmount?.trim()) {
return null;
}
let amount;
if (getNumberFormat().separatorRegex) {
amount = parseFloat(
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separatorRegex, '.'),
);
} else {
amount = parseFloat(
currencyAmount
.replace(getNumberFormat().regex, '')
.replace(getNumberFormat().separator, '.'),
);
}
return isNaN(amount) ? null : amount;
🧰 Tools
🪛 Biome (1.9.4)

[error] 410-410: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)

}

export function currencyToInteger(str: string) {
const amount = currencyToAmount(str);
export function currencyToInteger(
currencyAmount: CurrencyAmount,
): IntegerAmount | null {
const amount = currencyToAmount(currencyAmount);
return amount == null ? null : amountToInteger(amount);
}

export function stringToInteger(str: string) {
export function stringToInteger(str: string): number | null {
const amount = parseInt(str.replace(/[^-0-9.,]/g, ''));
if (!isNaN(amount)) {
return amount;
}
return null;
}
Comment on lines +420 to 426
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace global isNaN with Number.isNaN.

The global isNaN function attempts type coercion which can lead to unexpected results. Use Number.isNaN instead for safer number validation.

 export function stringToInteger(str: string): number | null {
   const amount = parseInt(str.replace(/[^-0-9.,]/g, ''));
-  if (!isNaN(amount)) {
+  if (!Number.isNaN(amount)) {
     return amount;
   }
   return null;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export function stringToInteger(str: string): number | null {
const amount = parseInt(str.replace(/[^-0-9.,]/g, ''));
if (!isNaN(amount)) {
return amount;
}
return null;
}
export function stringToInteger(str: string): number | null {
const amount = parseInt(str.replace(/[^-0-9.,]/g, ''));
if (!Number.isNaN(amount)) {
return amount;
}
return null;
}
🧰 Tools
🪛 Biome (1.9.4)

[error] 422-422: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.

See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.

(lint/suspicious/noGlobalIsNan)


export function amountToInteger(n: number) {
return Math.round(n * 100);
export function amountToInteger(amount: Amount): IntegerAmount {
return Math.round(amount * 100);
}

export function integerToAmount(n) {
return parseFloat((safeNumber(n) / 100).toFixed(2));
export function integerToAmount(integerAmount: IntegerAmount): Amount {
return parseFloat((safeNumber(integerAmount) / 100).toFixed(2));
}

// This is used when the input format could be anything (from
Expand Down
6 changes: 6 additions & 0 deletions upcoming-release-notes/4207.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
category: Maintenance
authors: [joel-jeremy]
---

Add type to the the amount utils to clarify what's the difference between amount, integer amount, and currency.
Loading