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

HelpScout 1199960 - Fixing intlFormat.ts error when currency is not defined #987

Merged
merged 4 commits into from
Aug 2, 2024
Merged

HelpScout 1199960 - Fixing intlFormat.ts error when currency is not defined #987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee05aa7
Ensure the currency is specified.
dr-bizz Aug 1, 2024
3c39a9c
Return something rather than error and show white screen of death
dr-bizz Aug 1, 2024
6ceaf0e
Adding tests
dr-bizz Aug 1, 2024
7c2846c
remove unwanted code
dr-bizz Aug 2, 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
7 changes: 7 additions & 0 deletions src/lib/intlFormat.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ describe('intlFormat', () => {
it('handles undefined case', () => {
expect(currencyFormat(1000, undefined, 'en-US')).toEqual('$1,000');
});
it('handles empty string case', () => {
expect(currencyFormat(1234.56, '', 'en-GB')).toEqual('US$1,234.56');
});

it('handles an error', () => {
expect(currencyFormat(1234.56, ' ', 'en-GB')).toEqual('1234.56 ');
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what happens if an absurd currency is given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is and it causes an error, we will revert to only returning the total amount with the currency in the text after it.
See line 36 on src/lib/intlFormat.ts

describe('different language', () => {
Expand Down
21 changes: 15 additions & 6 deletions src/lib/intlFormat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,21 @@ export const currencyFormat = (
): string => {
const amount = Number.isNaN(value) ? 0 : value;
const decimal = amount % 1 !== 0;
return new Intl.NumberFormat(locale, {
style: 'currency',
currency: currency ?? 'USD',
minimumFractionDigits: decimal ? 2 : 0,
maximumFractionDigits: decimal ? 2 : 0,
}).format(Number.isFinite(amount) ? amount : 0);
if (!currency) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get the same effect by giving it default argument of USD? I guess in that case we’d have to give locale a default as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what we had before with currency ?? 'USD', but for some reason it was still causing an error. Adding the if statement it prevent the error from happening.

currency = 'USD';
}
try {
return new Intl.NumberFormat(locale, {
style: 'currency',
currency: currency,
minimumFractionDigits: decimal ? 2 : 0,
maximumFractionDigits: decimal ? 2 : 0,
}).format(Number.isFinite(amount) ? amount : 0);
} catch (error) {
// eslint-disable-next-line no-console
console.error(`Error formatting currency: ${error}`);
return `${amount} ${currency}`;
}
};

export const dayMonthFormat = (
Expand Down
Loading