Repository navigation
Align CRUD type operations for consistent BigDecimal handling - #22
Conversation
ame-axelor
left a comment
There was a problem hiding this comment.
Fix looks correct — the new branch faithfully mirrors CreateArg's BigDecimal handling and resolves the asymmetry. Before merging, could you address a few things?
-
Add a test.
src/test/client-bigdecimal.test.tsexercisescreatewith string/number values but neverupdate. A small addition there would lock the contract in and catch any future regression:await client.country.update({ data: { id: country.id, version: country.version, population: "200.00" }, });
-
Extract a shared leaf alias to prevent drift. The leaf tails of
CreateArgandUpdateArgare now copy-pasted — if another special-cased leaf type lands later (Date, Buffer, etc.), it's easy to update one and forget the other. Something like:type LeafArg<T> = T extends BigDecimal ? AllowNull<string | number | bigint | T> : AllowNull<T>;
used in both
CreateArgandUpdateArgmakes the symmetry structural rather than convention-based. Optional but cheap. -
Flesh out the PR description. A one-line reproduction (e.g. "
client.X.update({ data: { decimalField: '1.23' }})failed to type-check while the equivalentcreateworks") would help anyone reading the history later understand what broke.
No concerns about correctness or runtime impact — happy to approve once the test is in.
Create a shared LeafArg<T> type that handles BigDecimal special-casing and null allowance. Use it in both CreateArg and UpdateArg to make the symmetry structural rather than convention-based, preventing future regressions if another special-cased leaf type lands.
Test that UpdateArg accepts string, number, bigint, and BigDecimal instances. Verifies the type fix resolves the inconsistency between create and update operations.
Update BulkSetArg to use LeafArg for consistency with CreateArg and UpdateArg. Add comprehensive bulk operation tests for BigDecimal fields using string, number, bigint, BigDecimal and null values.
38381a0 to
4776c03
Compare
|
All issues addressed, Let me know if further changes are required |
ame-axelor
left a comment
There was a problem hiding this comment.
All previous asks addressed and then some — LeafArg<T> extracted and applied to CreateArg, UpdateArg, and BulkSetArg (catching a third asymmetry I missed), plus round-trip tests covering string/number/bigint/BigDecimal/null for both update and updateAll. PR description rewritten with clear problem and solution. Nice work.
Note: the current build failure is unrelated to this PR — it's the Node 18 / pnpm 10.19 base-branch mismatch that PR #23 is addressing. This PR is purely type-level + tests and should pass once the CI upgrade lands.
Align CRUD type operations for consistent BigDecimal handling
Problem
The ORM CRUD types had inconsistent handling of BigDecimal fields:
Solution
1. Extract Shared Type (structural symmetry)
Created
LeafArg<T>type to eliminate copy-paste logic and prevent future drift:string | number | bigint | BigDecimalAllowNullfor nullable fieldsCreateArg,UpdateArg, andBulkSetArg2. Extend to Bulk Operations
Updated
BulkSetArg<T>to useLeafArg<T>, bringing parity with standard update operations3. Comprehensive Test Coverage
Changes
src/client/types/crud.ts: ExtractLeafArg<T>, apply toCreateArg,UpdateArg,BulkSetArgsrc/test/client-bigdecimal.test.ts: Comprehensive update test covering all value typessrc/test/client-bulk.test.ts: Bulk operation test mirroring update test pattern