[PUB-1667] Add support for encoding/decoding JSON objects for LiveMap values #2052
Conversation
WalkthroughThe changes update type definitions and implementation to allow JSON-serializable objects and arrays as valid LiveMap values. The code now uses a unified Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LiveMap
participant ObjectMessage
participant Wire
participant Client
User->>LiveMap: set(key, {foo: "bar"})
LiveMap->>ObjectMessage: create ObjectData with value={foo: "bar"}
ObjectMessage->>Wire: encode value as JSON string
Wire->>Client: send message with json field
Client->>Wire: receive message
Wire->>ObjectMessage: decode json field to object
ObjectMessage->>LiveMap: expose value as JS object
LiveMap->>User: get(key) returns {foo: "bar"}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)test/realtime/objects.test.jsNote ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learningstest/realtime/objects.test.js (3)Learnt from: VeskeR Learnt from: VeskeR Learnt from: VeskeR ⏰ 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). (7)
🔇 Additional comments (7)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
ably.d.ts (1)
2600-2606: PrimitiveObjectValue should stay in sync with the runtimeBufferlikeunion
PrimitiveObjectValuenow expands the value space to includeJsonArray | JsonObject, 👍.
However the binary branch diverges from the runtime definition found in
src/plugins/objects/objectmessage.ts(Bufferlike = Buffer | ArrayBuffer | Uint8Array).If consumer code relies on
Uint8Array, the current d.ts will raise a
type-error even though the runtime happily accepts it.-export type PrimitiveObjectValue = string | number | boolean | Buffer | ArrayBuffer | JsonArray | JsonObject; +import type {Bufferlike} from './src/plugins/objects/objectmessage'; // or declare the same alias here +export type PrimitiveObjectValue = string | number | boolean | Bufferlike | JsonArray | JsonObject;Keeping the two definitions aligned prevents downstream “type lies” and
removes the need for local// @ts-ignorework-arounds.
🧹 Nitpick comments (1)
src/plugins/objects/objectmessage.ts (1)
773-775: Consider more specific error handling for JSON parsing.While the JSON parsing is wrapped in a try-catch at the function level, malformed JSON could cause issues. Consider validating the string exists before parsing.
- let decodedJson: JsonObject | JsonArray | undefined; - if (objectData.encoding === 'json') { - decodedJson = JSON.parse(objectData.string!); // OD5a2, OD5b3 - } + let decodedJson: JsonObject | JsonArray | undefined; + if (objectData.encoding === 'json' && objectData.string != null) { + decodedJson = JSON.parse(objectData.string); // OD5a2, OD5b3 + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)ably.d.ts(4 hunks)src/common/lib/types/basemessage.ts(2 hunks)src/plugins/objects/livemap.ts(8 hunks)src/plugins/objects/objectmessage.ts(10 hunks)test/common/modules/objects_helper.js(1 hunks)test/realtime/objects.test.js(13 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
test/common/modules/objects_helper.js (2)
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
README.md (2)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
src/plugins/objects/livemap.ts (4)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
Learnt from: VeskeR
PR: ably/ably-js#1880
File: liveobjects.d.ts:26-26
Timestamp: 2024-10-08T15:37:26.765Z
Learning: For plugins like `LiveObjects`, the `any` type is intentionally used to maintain flexibility.
Learnt from: VeskeR
PR: ably/ably-js#1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In `src/plugins/liveobjects/liveobject.ts`, `EventEmitter` is accessed via the client object, so importing it with the `type` modifier is acceptable.
Learnt from: VeskeR
PR: ably/ably-js#1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as `test/common/modules/live_objects_helper.js`, concerns about assignments within expressions are not applicable.
test/realtime/objects.test.js (1)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as test/common/modules/live_objects_helper.js, concerns about assignments within expressions are not applicable.
</retrieved_learning>
ably.d.ts (3)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1917
File: src/plugins/liveobjects/liveobject.ts:2-2
Timestamp: 2024-11-08T05:28:30.955Z
Learning: In src/plugins/liveobjects/liveobject.ts, EventEmitter is accessed via the client object, so importing it with the type modifier is acceptable.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In src/plugins/liveobjects/statemessage.ts, using JSON.parse(JSON.stringify(...)) for deep copying is acceptable when buffer values are preserved in subsequent code.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1880
File: liveobjects.d.ts:26-26
Timestamp: 2024-10-08T15:37:26.765Z
Learning: For plugins like LiveObjects, the any type is intentionally used to maintain flexibility.
</retrieved_learning>
src/plugins/objects/objectmessage.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
🧬 Code Graph Analysis (3)
src/common/lib/types/basemessage.ts (1)
src/common/platform.ts (1)
Platform(18-42)
src/plugins/objects/livemap.ts (2)
ably.d.ts (3)
JsonArray(2620-2620)JsonObject(2625-2625)PrimitiveObjectValue(2605-2605)src/plugins/objects/objectmessage.ts (1)
PrimitiveObjectValue(26-26)
ably.d.ts (1)
src/plugins/objects/objectmessage.ts (1)
PrimitiveObjectValue(26-26)
🪛 GitHub Actions: Lint
README.md
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-npm-package
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (13)
test/common/modules/objects_helper.js (1)
88-95: LGTM! Good test coverage for JSON support and edge cases.The additions provide comprehensive test coverage for the new JSON object/array support:
- Edge case testing with maximum safe integers
- JSON object with proper
'json'encoding hint- JSON array with proper
'json'encoding hintThese test cases will help ensure the new LiveMap JSON functionality works correctly across different data types.
README.md (1)
691-699: LGTM! Examples effectively demonstrate the new JSON capabilities.The updated examples clearly showcase the new JSON object and array support while maintaining comprehensive coverage of other supported data types. The reorganization of keys (
'quux'→'grault'for counter, addition of'corge'for Uint8Array) maintains clarity in the examples.However, there's a prettier formatting issue that needs to be resolved:
#!/bin/bash # Fix prettier formatting issues in README.md prettier --write README.mdsrc/common/lib/types/basemessage.ts (2)
98-98: LGTM! Good simplification of the encoding API.Removing the
isNativeDataTypeparameter simplifies the function signature and moves the native data type detection logic intoencodeDatawhere it belongs.
114-118: LGTM! Centralized native data type detection.Moving the native data type check inside
encodeDatais a good refactoring that:
- Centralizes the logic in one location
- Removes the need for callers to perform this check
- Maintains identical behavior (checking for string, Buffer, null, or undefined)
The implementation correctly handles all the same native data types as before.
test/realtime/objects.test.js (6)
444-457: Excellent refactor to centralize map value validation logic.The
checkKeyDataOnMaphelper function is well-designed and handles all the necessary data types correctly:
- Buffer comparison using
BufferUtils.areBuffersEqualfor binary data- JSON parsing and deep comparison for JSON-encoded objects/arrays
- Primitive value comparison for strings, numbers, and booleans
This refactoring significantly improves test maintainability by eliminating code duplication across multiple test scenarios.
470-471: Good addition of JSON test data for comprehensive coverage.The new
objectKeyandarrayKeyentries properly demonstrate JSON object and array support:
- Valid JSON structures for testing
- Correct
encoding: 'json'metadata- Covers both objects and arrays as required by the PR objectives
This test data ensures the new JSON functionality is properly validated.
536-543: Correctly updated expected counts for new test data.The expected key counts and lists have been properly updated to account for the addition of
objectKeyandarrayKeytest entries. The size expectation of 13 correctly reflects all primitive values including the new JSON-encoded entries.Also applies to: 697-697
628-628: Proper implementation of JSON parsing for encoded values.The conditional JSON parsing logic correctly:
- Checks for
encoding === 'json'before attempting to parse- Uses
JSON.parse()to deserialize JSON strings into objects/arrays- Maintains consistency with existing data type handling patterns
This implementation properly supports the new JSON object/array functionality in LiveMap values.
Also applies to: 2628-2628, 2969-2970
989-995: Excellent refactoring of test assertions to use the centralized helper.The consistent replacement of inline assertion logic with
checkKeyDataOnMap()calls:
- Eliminates code duplication across multiple test scenarios
- Maintains the same validation behavior while improving maintainability
- Follows a consistent parameter pattern throughout
- Makes the test code cleaner and easier to understand
This refactoring significantly improves the test suite's code quality.
Also applies to: 1191-1197, 2121-2127, 2375-2381, 2640-2646, 2997-3003
716-735: Comprehensive test assertions for the new JSON value types.The new test assertions correctly validate:
maxSafeIntegerKeyandnegativeMaxSafeIntegerKeywith proper numeric valuesobjectKeyusingdeep.equalfor object comparison with expected{ foo: 'bar' }arrayKeyusingdeep.equalfor array comparison with expected['foo', 'bar', 'baz']The assertions properly test the JSON object and array support while maintaining consistent test patterns and descriptive error messages.
src/plugins/objects/livemap.ts (2)
27-30: LGTM! Good consolidation of primitive value fields.The unification of multiple primitive fields into a single
valuefield simplifies the data structure and makes it easier to extend with new types like JSON objects and arrays.
186-191: Good addition of null check!The explicit null check prevents a common JavaScript pitfall where
typeof null === 'object'. This ensures null values are properly rejected.src/plugins/objects/objectmessage.ts (1)
403-426: Well-structured encoding logic for different value types!The encoding properly handles all primitive types and adds JSON serialization for objects/arrays with appropriate encoding markers. This maintains backward compatibility while extending functionality.
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
b031dbb to
2af3822
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/realtime/objects.test.js (1)
470-471: Verify JSON test data covers edge cases.The new JSON test entries look good and provide coverage for both objects and arrays. Consider if additional edge cases should be tested, such as:
- Nested objects/arrays
- Empty objects
{}and arrays[]- Objects with special characters or unicode
- Arrays with mixed data types
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)ably.d.ts(4 hunks)src/plugins/objects/livemap.ts(8 hunks)src/plugins/objects/objectmessage.ts(10 hunks)test/common/modules/objects_helper.js(1 hunks)test/realtime/objects.test.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- test/common/modules/objects_helper.js
- README.md
- src/plugins/objects/livemap.ts
- src/plugins/objects/objectmessage.ts
- ably.d.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
test/realtime/objects.test.js (1)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as test/common/modules/live_objects_helper.js, concerns about assignments within expressions are not applicable.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (4)
test/realtime/objects.test.js (4)
444-457: LGTM! Well-designed helper function for centralized value verification.The
checkKeyDataOnMaphelper function properly handles all data types including the new JSON encoding. The logic correctly:
- Decodes bytes using base64
- Parses JSON when
encoding === 'json'- Handles primitive values appropriately
This centralization improves maintainability and ensures consistent verification across tests.
536-544: Consistent test expectations for expanded data types.The test expectations are correctly updated to account for the new JSON-encoded entries. The size expectations (line 697:
expect(valuesMap.size()).to.equal(13)) and individual value checks properly handle the newobjectKeyandarrayKeyentries.The deep equality checks for JSON objects and arrays are appropriate for ensuring the values are correctly parsed and stored.
Also applies to: 697-697, 716-735
989-995: Excellent refactoring to use the centralized helper function.All inline value verification logic has been consistently replaced with calls to
checkKeyDataOnMap. This standardizes the verification process and reduces code duplication. The helper function parameters are correctly passed in all instances.This refactoring improves maintainability and ensures consistent behavior across all tests.
Also applies to: 1191-1197, 2121-2127, 2375-2381, 2640-2646, 2997-3003
2628-2628: Correct JSON parsing logic for test value creation.The conditional JSON parsing logic is properly implemented:
value = keyData.data.encoding === 'json' ? JSON.parse(keyData.data.string) : keyData.data.string;This correctly handles both regular strings and JSON-encoded strings, ensuring test values are created with the appropriate data types for LiveMap operations.
Also applies to: 2969-2970
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
2af3822 to
3acefa2
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/realtime/objects.test.js (1)
450-452: Consider adding error handling for JSON parsing.While the JSON parsing logic is correct, consider wrapping
JSON.parse()in a try-catch block to provide more meaningful error messages if the test data contains invalid JSON.- } else if (keyData.data.encoding === 'json') { - const expectedObject = JSON.parse(keyData.data.string); - expect(mapObj.get(key)).to.deep.equal(expectedObject, msg); + } else if (keyData.data.encoding === 'json') { + let expectedObject; + try { + expectedObject = JSON.parse(keyData.data.string); + } catch (error) { + throw new Error(`Invalid JSON in test data for key "${key}": ${error.message}`); + } + expect(mapObj.get(key)).to.deep.equal(expectedObject, msg);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)ably.d.ts(4 hunks)src/common/lib/types/basemessage.ts(2 hunks)src/plugins/objects/livemap.ts(8 hunks)src/plugins/objects/objectmessage.ts(9 hunks)test/common/modules/objects_helper.js(1 hunks)test/realtime/objects.test.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- README.md
- test/common/modules/objects_helper.js
- src/common/lib/types/basemessage.ts
- ably.d.ts
- src/plugins/objects/livemap.ts
- src/plugins/objects/objectmessage.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
test/realtime/objects.test.js (2)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as test/common/modules/live_objects_helper.js, concerns about assignments within expressions are not applicable.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In src/plugins/liveobjects/statemessage.ts, using JSON.parse(JSON.stringify(...)) for deep copying is acceptable when buffer values are preserved in subsequent code.
</retrieved_learning>
🧬 Code Graph Analysis (1)
test/realtime/objects.test.js (1)
test/realtime/channel.test.js (16)
helper(155-155)helper(175-175)helper(213-213)helper(240-240)helper(270-270)helper(308-308)helper(343-343)helper(384-384)helper(414-414)helper(448-448)helper(488-488)helper(510-510)helper(543-543)helper(606-606)helper(661-661)helper(692-692)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (6)
test/realtime/objects.test.js (6)
444-458: LGTM! Well-structured helper function with good separation of concerns.The new
checkKeyDataOnMaphelper function effectively unifies the verification logic for different data types (bytes, JSON, primitives) and improves test maintainability. The function correctly handles the three distinct cases and provides clear error messaging.
470-471: Excellent addition of JSON test data.The new
objectKeyandarrayKeyentries properly demonstrate JSON object and array support with correct encoding specification. This aligns perfectly with the PR objectives.
697-697: Correct update to expected map size.The increase from 9 to 13 keys properly reflects the addition of the new JSON-encoded test entries (
objectKeyandarrayKey) plus the existing keys.
989-996: Good refactoring to use the unified helper function.The replacement of manual assertions with calls to
checkKeyDataOnMapstandardizes the test approach and improves maintainability across multiple test scenarios.Also applies to: 1191-1198, 2121-2128, 2375-2382, 2640-2647, 2997-3004
728-735: Comprehensive JSON object and array validation.The test properly validates that JSON objects and arrays are correctly decoded and accessible via the LiveMap API. The deep equality checks ensure the data integrity is maintained.
2628-2628: Consistent JSON parsing implementation.The conditional JSON parsing logic correctly handles the new encoding type while maintaining backward compatibility with existing string values.
Also applies to: 2969-2970
…cts as LiveMap values ably-js support for this added in [1] [1] ably/ably-js#2052
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
3acefa2 to
c6fec17
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/plugins/objects/objectmessage.ts (1)
789-792: Fix unsafe type casting in error fallback.The error fallback casts
WireObjectDatatoObjectData, but these types have different structures after the refactoring.WireObjectDatahas separate primitive fields whileObjectDatahas a unifiedvaluefield.Replace the unsafe casting with proper error handling:
- return { - ...objectData, - } as ObjectData; + return { + objectId: objectData.objectId, + value: objectData.string ?? objectData.number ?? objectData.boolean ?? undefined, + };
🧹 Nitpick comments (2)
src/plugins/objects/objectmessage.ts (2)
773-776: JSON parsing should handle errors more specifically.The JSON.parse operation could throw but relies on the outer try-catch. Consider adding more specific error handling for JSON parsing failures.
- if (objectData.encoding === 'json') { - decodedJson = JSON.parse(objectData.string!); // OD5a2, OD5b3 - } + if (objectData.encoding === 'json') { + try { + decodedJson = JSON.parse(objectData.string!); // OD5a2, OD5b3 + } catch (jsonError) { + client.Logger.logAction( + client.logger, + client.Logger.LOG_ERROR, + 'WireObjectMessage._decodeObjectData() - JSON parse error', + this._utils.inspectError(jsonError), + ); + } + }
408-425: Verify buffer vs JSON object handling precedence.The JSON encoding logic looks correct, but the object type check on line 421 could potentially match Buffer objects. While Buffer handling occurs first (line 408), the code flow would be clearer with an explicit exclusion.
Consider making the JSON object check more explicit:
- if (typeof data.value === 'object' && data.value !== null) { + if (typeof data.value === 'object' && data.value !== null && !client.Platform.BufferUtils.isBuffer(data.value)) {#!/bin/bash # Check if there are any Buffer-related test cases that might conflict with JSON object handling rg -A 5 -B 5 "BufferUtils.*isBuffer" --type ts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)ably.d.ts(4 hunks)src/plugins/objects/livemap.ts(8 hunks)src/plugins/objects/objectmessage.ts(9 hunks)test/common/modules/objects_helper.js(1 hunks)test/realtime/objects.test.js(15 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- test/common/modules/objects_helper.js
- src/plugins/objects/livemap.ts
- ably.d.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
test/realtime/objects.test.js (3)
undefined
<retrieved_learning>
Learnt from: VeskeR
PR: #1909
File: test/common/modules/live_objects_helper.js:209-209
Timestamp: 2024-10-25T08:54:46.022Z
Learning: In test code files such as test/common/modules/live_objects_helper.js, concerns about assignments within expressions are not applicable.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In src/plugins/liveobjects/statemessage.ts, using JSON.parse(JSON.stringify(...)) for deep copying is acceptable when buffer values are preserved in subsequent code.
</retrieved_learning>
<retrieved_learning>
Learnt from: VeskeR
PR: #1897
File: test/realtime/live_objects.test.js:125-127
Timestamp: 2024-10-22T16:20:01.724Z
Learning: In the file test/realtime/live_objects.test.js, the field name regionalTimeserial is correct and should not be changed to timeserial.
</retrieved_learning>
src/plugins/objects/objectmessage.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1924
File: src/plugins/liveobjects/statemessage.ts:220-224
Timestamp: 2024-11-22T17:15:25.075Z
Learning: In `src/plugins/liveobjects/statemessage.ts`, using `JSON.parse(JSON.stringify(...))` for deep copying is acceptable when buffer values are preserved in subsequent code.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
🔇 Additional comments (12)
test/realtime/objects.test.js (8)
444-457: LGTM! Well-designed helper function for centralized validation.The
checkKeyDataOnMaphelper function effectively consolidates the validation logic for different data types including the new JSON encoding support. The implementation correctly handles bytes decoding, JSON parsing whenencoding === 'json', and primitive value comparison.
470-471: LGTM! Appropriate test data for JSON encoding support.The addition of
objectKeyandarrayKeyentries with JSON-encoded strings properly extends the test fixtures to validate the new JSON functionality. The encoding field correctly identifies these as JSON-encoded values.
536-543: LGTM! Test expectations properly updated for JSON functionality.The test assertions correctly:
- Update map size expectations to account for new JSON-encoded keys
- Validate that JSON-encoded objects and arrays are properly parsed and stored as native JavaScript values
- Use appropriate deep equality comparisons for complex data types
Also applies to: 697-697, 716-735
989-995: LGTM! Consistent usage of helper function improves maintainability.The systematic replacement of inline validation logic with the
checkKeyDataOnMaphelper function:
- Reduces code duplication across multiple test scenarios
- Provides consistent validation behavior
- Improves readability and maintainability
- Maintains appropriate error messages for test failures
Also applies to: 1191-1197, 2121-2127, 2375-2381, 2640-2646, 2997-3003
628-628: LGTM! Correct JSON parsing logic.The conditional logic properly checks for
encoding === 'json'before parsing, ensuring that only JSON-encoded strings are parsed while regular strings are handled normally. This matches the pattern established in the helper function.
969-970: LGTM! Consistent JSON handling in map creation tests.The JSON parsing logic in the
Objects.createMaptest correctly follows the established pattern, ensuring proper handling of both regular strings and JSON-encoded values during object creation.
4789-4823: LGTM! Comprehensive message size tests for JSON payloads.The new test cases for object and array payloads properly validate message size calculations for JSON-encoded data:
- Correctly use
JSON.stringify()to create the encoded data- Include the
encoding: 'json'field appropriately- Calculate expected sizes using the serialized JSON string length
- Provide good coverage for both object and array data types
4717-4717: LGTM! Minor documentation improvement.The updated test description provides clearer context for the test case.
src/plugins/objects/objectmessage.ts (4)
5-36: LGTM! Type definitions properly support JSON functionality.The import of JSON types and the unified
PrimitiveObjectValuetype definition correctly support the new JSON object/array functionality. The simplification ofObjectDatato use a singlevaluefield is a good architectural improvement.
189-190: Good defensive programming with optional encoding function.Making
encodeInitialValueFnoptional with proper fallback logic ensures backward compatibility while supporting the enhanced functionality.Also applies to: 210-213, 226-229
764-771: Buffer decoding logic correctly handles different wire formats.The conditional logic properly handles msgpack (bytes already decoded) vs JSON (base64 decoding required) formats.
7-7: Verify function signature changes don't break existing usage.The simplification of
EncodeInitialValueFunctionfrom multiple parameters to a single parameter is a breaking change that could affect other parts of the codebase.#!/bin/bash # Search for usage of EncodeInitialValueFunction to verify compatibility rg -A 3 -B 3 "EncodeInitialValueFunction" --type ts # Also check for any direct function calls that might use the old signature ast-grep --pattern 'encodeInitialValueFn($_, $_, $_)'Also applies to: 498-501
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
c6fec17 to
6366a52
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
|
@VeskeR I think it was mentioned in a spec PR, but thought I'd mention here too, it looks like this sets the JSON encoded value to the Then a JSON encoded value would look like this over the wire: { "json": "{\"a\": \"json object\"}" }
{ "json": "[\"a\", \"json\", \"array\"]" }rather than: { "string": "{\"a\": \"json object\"}", "encoding": "json"}
{ "string": "[\"a\", \"json\", \"array\"]", "encoding": "json" } |
|
@lmars As discussed, I've changed the implementation to use I've also added you as a reviewer. Mike has already reviewed and approved the preivous set of changes so you can focus on the |
This enables to set and get JSON objects and arrays via LiveMap.set and LiveMap.get methods. Resolves PUB-1667
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
42ac88b to
fe088b3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
fe088b3 to
e6ce071
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2052/bundle-report
…leaf values in Objects This adds client side support for the realtime change made in [1] [1] ably/realtime#7606
VeskeR
force-pushed
the
PUB-1667/map-set-objects
branch
from
e6ce071 to
6d43429
Compare
Public API based on [1] at 6d43429. [1] ably/ably-js#2052
Public API based on [1] at 6d43429. [1] ably/ably-js#2052
Public API based on [1] at 6d43429. [1] ably/ably-js#2052
…cts as LiveMap values ably-js support for this added in [1] [1] ably/ably-js#2052
…cts as LiveMap values ably-js support for this added in [1] [1] ably/ably-js#2052
This enables users to set and get JSON objects and arrays via
LiveMap.setandLiveMap.getmethods.Docs PR for this ably/docs#2689.
Resolves PUB-1667
Summary by CodeRabbit
New Features
Bug Fixes
Tests