Switch JSONValue's number case back to Double
#91
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReworks numeric handling by introducing a Number generic to ExtendedJSONValue and switching JSONValue numbers to Double. Updates serialization, deserialization, and mapping APIs to accept number transformers. WireValue bridges numbers via NSNumber. Tests adjust to Double-based numbers and JavaScript’s MAX_SAFE_INTEGER boundaries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant JS as JSONSerialization
participant JV as JSONValue (Double)
participant EJV as ExtendedJSONValue<Number, Extra>
participant WV as WireValue (NSNumber)
Note over EJV: New: Number generic parameter
JS->>EJV: init(deserialized:any, createNumberValue, createExtraValue)
EJV-->>JV: map(number: to Double, extra: drop) -> ExtendedJSONValue<Double, Never>
JV-->>JS: toJSONSerializationInputElement (serializeNumberValue: Double→NSNumber)
rect rgba(220,240,255,0.4)
JV->>EJV: toExtendedJSONValue()
EJV-->>WV: map(number: Double→NSNumber, extra: passthrough)
WV-->>EJV: toExtendedJSONValue() (NSNumber retained)
EJV-->>JV: map(number: NSNumber.doubleValue, extra: drop)
end
Note over JV,WV: Numbers: JV=Double, WV=NSNumber, EJV=generic Number
sequenceDiagram
participant In as Input Any
participant E as ExtendedJSONValue<N, X>
participant Out as Serialized Any
In->>E: init(deserialized:, createNumberValue, createExtraValue)
E->>Out: serialized(serializeNumberValue, serializeExtraValue)
Note over E: .number uses provided closures throughout recursion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
lawrence-forooghian
changed the title
number case back to Doublenumber case back to Double
lawrence-forooghian
changed the title
number case back to DoubleJSONValue's number case back to Double
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
github-actions
bot
temporarily deployed
to
staging/pull/91/AblyLiveObjects
![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
🧹 Nitpick comments (4)
Sources/AblyLiveObjects/Utility/WireValue.swift (1)
238-249: Precision loss when converting NSNumber → Double (int > 2^53).This is expected after reverting JSONValue.number to Double, but it’s worth guarding in debug builds to surface accidental oversized integers.
Apply an optional debug assertion to flag values beyond JS safe integer range:
- let neverExtended = try toExtendedJSONValue.map(number: { (number: NSNumber) throws(ARTErrorInfo) -> Double in - number.doubleValue + let neverExtended = try toExtendedJSONValue.map(number: { (number: NSNumber) throws(ARTErrorInfo) -> Double in + let d = number.doubleValue + #if DEBUG + let maxSafeInteger = 9_007_199_254_740_991.0 // 2^53 - 1 + if abs(d) > maxSafeInteger { + assertionFailure("Precision loss: converting NSNumber (\(number)) to Double beyond JS safe integer range") + } + #endif + return d }, extra: { (extra: ExtraValue) throws(ARTErrorInfo) -> Never inAlso, please confirm that NSNumber’s Sendable conformance on your minimum deployment targets/toolchain is sufficient for WireValue: Sendable. If not, we may need to revisit Sendable here or wrap NSNumber accesses.
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
70-93: map(number:extra:) redesign is clear and ergonomic.Typed throws and separate transformers for number/extra are well done.
Consider adding lightweight typealiases for common instantiations to improve readability at call sites:
- typealias JSONExtended = ExtendedJSONValue<Double, Never>
- typealias WireExtended = ExtendedJSONValue<NSNumber, WireValue.ExtraValue>
Sources/AblyLiveObjects/Utility/JSONValue.swift (2)
113-115: Integer literal to Double: clarify precision limits.Large integers (> 2^53-1) won’t round‑trip exactly as
Double. Consider documenting this on the enum or the integer-literal init.
154-155: Prefer explicit NSNumber construction for serialization (and optionally guard non‑finite).Use
NSNumber(value:)to avoid brittle bridging and make intent explicit. Optionally, reject non‑finite values early.Apply this diff:
- toExtendedJSONValue.serialized(serializeNumberValue: { $0 as NSNumber }, serializeExtraValue: { _ in }) + toExtendedJSONValue.serialized( + serializeNumberValue: { NSNumber(value: $0) }, + serializeExtraValue: { _ in } + )Optional stricter version:
- toExtendedJSONValue.serialized(serializeNumberValue: { $0 as NSNumber }, serializeExtraValue: { _ in }) + toExtendedJSONValue.serialized( + serializeNumberValue: { + precondition($0.isFinite, "Non-finite JSON number is not permitted") + return NSNumber(value: $0) + }, + serializeExtraValue: { _ in } + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift(5 hunks)Sources/AblyLiveObjects/Utility/JSONValue.swift(7 hunks)Sources/AblyLiveObjects/Utility/WireValue.swift(7 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift(2 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(6 hunks)Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: lint
- GitHub Check: Generate code coverage
🔇 Additional comments (19)
Sources/AblyLiveObjects/Utility/WireValue.swift (5)
6-14: Good rationale for using NSNumber on the wire.Using NSNumber here to preserve integer semantics for MessagePack encoding is the right call, while keeping JSONValue as Double.
125-135: Bridging from plugin support data looks correct.Using ExtendedJSONValue<NSNumber, ExtraValue> with createNumberValue: { $0 } and explicit Data handling is clean and consistent.
152-158: Serialization path is consistent with the new ExtendedJSONValue API.serializeNumberValue passes through NSNumber as intended, and extra values handle Data only.
177-197: Clean bidirectional mapping with ExtendedJSONValue<NSNumber, ExtraValue>.The init and toExtendedJSONValue cases are exhaustive and correct.
Also applies to: 199-216
224-227: JSONValue → WireValue bridging is correct.Mapping Double to NSNumber via map(number:) aligns with the new generics.
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (3)
3-12: Solid abstraction: parameterizing Number separately from Extra.This cleanly enables Double for JSON and NSNumber for wire without duplicating logic.
20-42: Correct NSNumber/Bool discrimination during deserialization.The kCFBooleanTrue/False identity check avoids the classic NSNumber(0/1) vs Bool pitfall.
47-64: Serialization hooks for numbers/extras are appropriate.Propagating serializeNumberValue through nested structures keeps callers in control (NSNumber vs Double).
Please confirm that upstream JSON serialization paths reject or sanitize NaN/±Infinity (e.g., for JSONValue/Double). If not, consider guarding before invoking JSONSerialization to avoid runtime failures or invalid JSON.
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (1)
454-455: LGTM: JSON REST ops now use Double-based JSONValue.Switching to .number(Double) for REST request bodies matches the public API revert.
Double-check all callers of counterCreateRestOp/counterIncRestOp were updated to pass Double (not NSNumber/Int) to avoid implicit conversions.
Also applies to: 470-471
Tests/AblyLiveObjectsTests/ObjectCreationHelpersTests.swift (1)
68-69: LGTM: Test expectation updated to Double.Expecting semantics as Double aligns with JSONValue.number(Double).
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (3)
134-136: Good addition: Number.MAX_SAFE_INTEGER analogue.Using 2^53−1 avoids accidental precision surprises in Double-backed numbers.
161-169: LGTM: Fixtures use maxSafeInteger and its negative.Works well with Double-backed JSON values.
860-861: LGTM: Compare numberValue directly.Switching from doubleValue to numberValue matches the JSONValue API change and keeps assertions straightforward.
Also applies to: 1076-1077, 2053-2054, 2353-2354
Sources/AblyLiveObjects/Utility/JSONValue.swift (6)
32-32: Revert to Double for numbers looks good; ensure semver/migration note.Public API is restored to
Double. Please add a short migration note (calls that usedNSNumbernow expectDouble) and ensure the next release versioning reflects this source change.
66-72:numberValue->Double?LGTM.Type and accessor semantics are consistent with the enum case.
119-121: Float literal mapping is correct.No concerns.
139-145: Deserialization bridge viadoubleValueis appropriate.Cleanly captures
NSNumbertoDoublewithout changing control flow.
228-243: ExtendedJSONValue bridging initializer reads cleanly.Cases are mapped 1:1; no hidden behavior changes.
245-260: Conversion back to ExtendedJSONValue is correct.Number path now returns
Double; object/array recursion intact.
lawrence-forooghian
force-pushed
the
39-less-passing-LiveMap-delegate
branch
from
827652c to
96799cb
Compare
This reverts the public API change of fd1e91c whilst preserving the internal MessagePack-related motivation for that commit.
lawrence-forooghian
force-pushed
the
switch-JSONValue-number-back-to-Double
branch
from
c891a47 to
355497e
Compare
Note: This PR is based on top of #88; please review that one first.
This reverts the public API change of fd1e91c whilst preserving the internal MessagePack-related motivation for that commit.
Summary by CodeRabbit