chore: allow string or number amount in format price#2
Conversation
WalkthroughWidened Currency.amount type to accept number in addition to string and updated formatPrice to handle numeric amounts directly. Adjusted unit tests to use numeric amounts and added a parity test ensuring identical formatting for string vs number inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant F as formatPrice
participant P as Intl.NumberFormat
C->>F: formatPrice({ amount, currencyCode, ... })
alt amount is number
Note right of F: Use amount directly
else amount is string
F->>F: parseFloat(amount)
Note right of F: Convert to numeric price
end
F->>P: format(price, options)
P-->>F: formatted string
F-->>C: return formatted price
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/currency/index.ts (1)
14-23: Guard parsing and base “no-cents” on the final amount, not the unit price
- parseFloat is permissive (“12.3abc” → 12.3) and may yield NaN silently.
- The cents toggle checks
price % 1but formatting usesprice * quantity; this misbehaves for non-integer quantities or when floating error creeps in.Apply:
- const price = typeof amount === 'number' ? amount : Number.parseFloat(amount); - - return new Intl.NumberFormat(locale, { + const price = + typeof amount === 'number' ? amount : Number(amount); + if (!Number.isFinite(price)) { + throw new TypeError(`Invalid amount: ${amount as unknown as string}`); + } + const total = price * quantity; + const isIntegerTotal = + Number.isInteger(total) || Math.abs(total - Math.round(total)) < 1e-9; + + return new Intl.NumberFormat(locale, { style: 'currency', currency: currencyCode, // eslint-disable-next-line @typescript-eslint/no-magic-numbers - maximumFractionDigits: disableCents && price % 1 === 0 ? 0 : 2, + maximumFractionDigits: disableCents && isIntegerTotal ? 0 : 2, // eslint-disable-next-line @typescript-eslint/no-magic-numbers - minimumFractionDigits: disableCents && price % 1 === 0 ? 0 : 2, - }).format(price * quantity); + minimumFractionDigits: disableCents && isIntegerTotal ? 0 : 2, + }).format(total);
🧹 Nitpick comments (2)
src/currency/index.ts (1)
16-23: Optional: respect currency-specific fraction digitsIf you want JPY/CLP (0 decimals) or TND (3 decimals) to follow CLDR defaults, omit explicit min/max digits when
disableCentsis false, or derive them fromIntl.NumberFormat(...).resolvedOptions().src/currency/index.spec.ts (1)
6-19: Optional: validate bad string inputsIf you adopt input validation (throw on non-numeric strings), add a negative test.
Add:
test('throws on invalid amount string', () => { const money = { amount: '12.34abc', currencyCode: 'USD' } satisfies Currency; expect(() => formatPrice(money, 'en-US')).toThrow(TypeError); });If localized strings (e.g., '1 234,56') must be accepted, we should normalize per-locale before parsing and add tests accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/currency/index.spec.ts(1 hunks)src/currency/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/currency/index.spec.ts (1)
src/currency/index.ts (2)
Currency(3-6)formatPrice(8-24)
🔇 Additional comments (3)
src/currency/index.ts (1)
4-4: Type widening to support numbers looks goodNon-breaking for TS consumers and improves ergonomics.
src/currency/index.spec.ts (2)
6-19: Parity test (string vs number) is solidGood guard to ensure behavior remains identical.
41-46: Add a test for disableCents with non-integer quantitiesCurrent implementation bases the cents decision on the unit price, not the final total; the test below will catch that and validate the proposed fix.
Add:
test('disableCents bases decision on final total', () => { const money = { amount: 10, currencyCode: 'USD' } satisfies Currency; expect(formatPrice(money, 'en-US', 1.5, true)).toBe('$15.00'); // cents shown expect(formatPrice(money, 'en-US', 2, true)).toBe('$20'); // no cents });
2 participants
Summary by CodeRabbit
New Features
Tests