[PUB-1672] Refactor encoding/decoding for ObjectMessage
#2051
Conversation
VeskeR
changed the title
ObjectMessage
WalkthroughThis change introduces a clear separation between wire-format and decoded object messages by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealtimeChannel
participant ObjectsPlugin
participant WireObjectMessage
participant ObjectMessage
Client->>RealtimeChannel: sendState(WireObjectMessage[])
RealtimeChannel->>ObjectsPlugin: processMessage(WireObjectMessage[])
ObjectsPlugin->>WireObjectMessage: decode(client, format)
WireObjectMessage-->>ObjectMessage: returns decoded ObjectMessage
ObjectsPlugin->>ObjectsPlugin: handleDecodedObjectMessages(ObjectMessage[])
Estimated code review effort3 (~45 minutes) Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ 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)
✨ 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/2051/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: 2
🧹 Nitpick comments (1)
src/plugins/objects/objectmessage.ts (1)
575-582: Document the format inference logic in toJSONThe logic to infer format from
arguments.lengthis clever but non-obvious. This relies on implementation details of how JSON.stringify and msgpack call toJSON.Add a more detailed comment explaining this behavior:
toJSON() { - // we can infer the format used by client by inspecting with what arguments this method was called. - // if JSON protocol is being used, the JSON.stringify() will be called and this toJSON() method will have a non-empty arguments list. - // MSGPack protocol implementation also calls toJSON(), but with an empty arguments list. + // Format inference based on how toJSON is called: + // - JSON.stringify() calls toJSON(key) with the property key, resulting in arguments.length > 0 + // - MSGPack's implementation calls toJSON() without arguments, resulting in arguments.length === 0 + // This behavior is relied upon but could break if the calling implementations change. const format = arguments.length > 0 ? this._utils.Format.json : this._utils.Format.msgpack;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/common/lib/client/realtimechannel.ts(3 hunks)src/common/lib/types/protocolmessage.ts(3 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(11 hunks)src/plugins/objects/livemap.ts(20 hunks)src/plugins/objects/liveobject.ts(4 hunks)src/plugins/objects/objectmessage.ts(20 hunks)src/plugins/objects/objects.ts(2 hunks)src/plugins/objects/syncobjectsdatapool.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/common/lib/client/realtimechannel.ts (1)
src/plugins/objects/objectmessage.ts (1)
WireObjectMessage(427-786)
src/common/lib/types/protocolmessage.ts (1)
src/plugins/objects/objectmessage.ts (1)
WireObjectMessage(427-786)
src/plugins/objects/syncobjectsdatapool.ts (1)
src/plugins/objects/objectmessage.ts (2)
ObjectState(169-192)ObjectData(32-46)
src/plugins/objects/liveobject.ts (1)
src/plugins/objects/objectmessage.ts (4)
ObjectOperation(130-163)ObjectData(32-46)ObjectMessage(333-420)ObjectState(169-192)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (15)
src/plugins/objects/index.ts (1)
1-10: LGTM! Clean export addition for the new WireObjectMessage type.The addition of
WireObjectMessageto both the import statement and exports maintains consistency with the existing export pattern.src/common/lib/types/protocolmessage.ts (3)
71-78: Excellent type consistency for wire-format message handling.The update from
ObjectMessage[]toWireObjectMessage[]and the correspondingfromValuesArraycall properly aligns the protocol message layer with the new wire-format representation.
131-131: Consistent update in stringify function.The change to use
WireObjectMessage.fromValuesArraymaintains consistency with the wire-format approach used throughout the protocol message handling.
172-172: Proper type declaration for the state property.The
WireObjectMessage[]type correctly represents the wire-format messages stored in protocol messages, aligning with the broader refactoring objectives.src/plugins/objects/objects.ts (1)
308-320: Well-implemented encoding workflow in the publish method.The changes properly implement the separation between
ObjectMessageandWireObjectMessage:
- Line 308: Each
ObjectMessageis encoded using its instance methodencode(client)to produceWireObjectMessageinstances- Line 310: Size calculation correctly uses the encoded messages (wire format) rather than the original messages
- Line 319: The channel receives properly encoded
WireObjectMessageinstancesThis maintains a clean separation where
ObjectMessagerepresents the internal API andWireObjectMessagerepresents the wire transmission format.src/common/lib/client/realtimechannel.ts (3)
20-20: Correct import update for wire-format handling.The import change from
ObjectMessagetoWireObjectMessageproperly reflects that the realtime channel handles wire-format messages during transmission and reception.
510-517: Proper type signature for wire-format message transmission.The
sendStatemethod now correctly acceptsWireObjectMessage[]which aligns with the separation of concerns where channels handle encoded wire-format messages.
631-631: Correct decoding implementation for received messages.The change to
message.state.map((om) => om.decode(this.client, format))properly converts receivedWireObjectMessageinstances toObjectMessageinstances for internal processing. The use of the active transport format ensures proper decoding based on the actual protocol in use (JSON vs MsgPack).src/plugins/objects/syncobjectsdatapool.ts (3)
3-3: Good addition of ObjectData import for improved typing.Adding the
ObjectDataimport enables more specific generic parameterization ofObjectStatethroughout the file.
7-7: Enhanced type safety with ObjectState generic parameter.The change from
ObjectStatetoObjectState<ObjectData>improves type specificity and ensures the object state is properly typed according to theObjectDatainterface structure.
81-97: Consistent generic parameterization in private methods.Both
_createLiveCounterDataEntryand_createLiveMapDataEntrymethods now useObjectState<ObjectData>which maintains type consistency throughout the sync object data pool implementation.src/plugins/objects/liveobject.ts (1)
3-3: Type refinements look good!The parameterization of
ObjectOperationandObjectStatewithObjectDataenhances type safety and aligns with the separation between wire-format and decoded messages introduced in this PR.Also applies to: 222-222, 235-235, 256-256
src/plugins/objects/livecounter.ts (1)
3-11: Consistent type updates and proper use of modular encoding!The changes correctly:
- Import and use the new
encodeInitialValuefunction instead of the static method- Apply generic parameterization consistently across all method signatures
- Update type casts to match the new generic types
Also applies to: 40-40, 52-52, 74-74, 94-94, 115-115, 127-127, 151-151, 173-173, 180-180, 237-237, 311-311, 322-322, 330-330
src/plugins/objects/objectmessage.ts (1)
48-66: Well-designed wire format interface!The
WireObjectDatainterface correctly handles the dual nature of the bytes field - asBufferlikein binary protocols and as base64-encodedstringin JSON protocol. This design clearly separates wire representation from the decoded format.src/plugins/objects/livemap.ts (1)
9-9: Clean refactoring with improved type clarity!Good choice renaming the type alias to
LiveMapObjectDatato avoid confusion with the importedObjectDatatype. The generic parameterization is applied consistently throughout the file.Also applies to: 41-41, 87-93, 102-108, 227-227
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
97ac01e to
5f3788b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
5f3788b to
516acca
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/plugins/objects/objectmessage.ts (1)
577-584: Error handling allows partial decoding which could cause downstream issues.The decode method catches errors and logs them but continues execution, which could lead to partially decoded messages with missing fields. This might cause unexpected behavior downstream.
Consider either throwing the error after logging to fail fast, or adding validation after decoding to ensure critical fields were decoded successfully.
🧹 Nitpick comments (1)
src/plugins/objects/objectmessage.ts (1)
265-304: Consider simplifying the multi-step encoding process.The function performs multiple encoding transformations:
ObjectMessage→WireObjectMessage→encodeForWire→encodeBody. While this ensures proper data encoding, it creates temporary objects and multiple transformations that could potentially be optimized.Consider whether some of these steps can be combined or if the encoding flow can be streamlined without losing correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/common/lib/client/realtimechannel.ts(3 hunks)src/common/lib/types/protocolmessage.ts(3 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(11 hunks)src/plugins/objects/livemap.ts(20 hunks)src/plugins/objects/liveobject.ts(4 hunks)src/plugins/objects/objectmessage.ts(20 hunks)src/plugins/objects/objects.ts(2 hunks)src/plugins/objects/syncobjectsdatapool.ts(3 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/plugins/objects/index.ts
- src/common/lib/types/protocolmessage.ts
- src/plugins/objects/objects.ts
- src/plugins/objects/syncobjectsdatapool.ts
- src/plugins/objects/liveobject.ts
- src/common/lib/client/realtimechannel.ts
- test/realtime/objects.test.js
- src/plugins/objects/livecounter.ts
- src/plugins/objects/livemap.ts
🧰 Additional context used
🧠 Learnings (1)
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-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-npm-package
🔇 Additional comments (6)
src/plugins/objects/objectmessage.ts (6)
48-66: Excellent separation of wire format from decoded format.The introduction of
WireObjectDatainterface provides clear type safety distinction between wire format (wherebytescan be a string) and decoded format (wherebytesis always a buffer). This architectural improvement enhances type safety and makes the encoding/decoding flow explicit.
194-239: Well-structured encoding implementation.The encoding function properly handles the buffer copying limitation by using the original message for encoding operations while working with a deep copy for structure modification. The comprehensive field handling ensures all object data is properly encoded.
329-350: Deep copying approach is acceptable for this use case.The use of
JSON.parse(JSON.stringify())for deep copying is appropriate here since buffer values are preserved and used from the original message in subsequent encoding operations, as indicated in the retrieved learnings.
385-444: Clean architectural separation achieved.The
ObjectMessageclass now properly represents decoded messages withObjectDatatypes, and theencode()method cleanly returns aWireObjectMessage. This separation of concerns improves code clarity and type safety.
446-537: Well-implemented wire format handling.The
WireObjectMessageclass properly handles wire format encoding with protocol-specific logic for JSON vs msgpack. The static factory methods andencodeForWiremethod provide clean interfaces for message creation and encoding.
754-811: Robust decoding implementation with proper protocol handling.The private decoding methods properly handle the differences between JSON and msgpack protocols, with appropriate base64 decoding for JSON protocol and direct buffer handling for msgpack. The error handling in
_decodeObjectDataprovides a reasonable fallback.
| * Deep copy public properties of object message. | ||
| * Buffer values won't be correctly copied, use the original message instead if you need buffer values. | ||
| */ | ||
| function copyMsg( |
There was a problem hiding this comment.
I had a go at adding the BufferUtils.copy method and then updating the copyMsg function to also copy buffer values. I think it actually made things more complicated. Now we would need to iterate over every map entry and map operation in the object and operation fields within the copyMsg function as well (which we’re already doing in the decode and encode calls anyway and it's not going away). Additionally, the function becomes dependent on the core library's Platform class to get access to BufferUtils, which would need to be passed around a lot and added to the ObjectMessage and WireObjectMessage constructors.
Essentially, this just introduced more complexity and additional lines of code without providing much benefit. The copyMsg function isn’t even exported - it's used only within this file in two specific cases - and it already clearly states that buffer values are not copied. I don’t think there’s much value in trying to handle buffer copying here.
I've added a bit more detais to the function's jsdoc to clarify what's going on: https://github.com/ably/ably-js/compare/7d2e10530771b99cc5861864639dc176b006df03..3a332be1bad7f7afa04a07bb78fceb6b4e6c8bd8
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
516acca to
5c25961
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/plugins/objects/objectmessage.ts (2)
72-76: Consider renaming generic type parameter for clarity.Throughout the generic interfaces,
TDatacould be renamed toDatafor better readability and to clearly communicate its purpose.Also applies to: 92-104, 110-115, 130-163, 169-192
585-592: Improve error handling in decode method.The error is logged but decoding continues, which could lead to partially decoded messages with missing fields. This could cause unexpected behavior downstream.
🧹 Nitpick comments (1)
src/plugins/objects/objectmessage.ts (1)
11-11: Consider more descriptive type parameter naming.The generic type parameter
TDatacould be renamed to something more descriptive likeDatato clearly communicate its purpose, as suggested in past reviews.-export type EncodeObjectDataFunction = (data: ObjectData | WireObjectData) => WireObjectData; +export type EncodeObjectDataFunction = (data: ObjectData | WireObjectData) => WireObjectData;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/common/lib/client/realtimechannel.ts(3 hunks)src/common/lib/types/protocolmessage.ts(3 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(11 hunks)src/plugins/objects/livemap.ts(20 hunks)src/plugins/objects/liveobject.ts(4 hunks)src/plugins/objects/objectmessage.ts(20 hunks)src/plugins/objects/objects.ts(2 hunks)src/plugins/objects/syncobjectsdatapool.ts(3 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/plugins/objects/index.ts
- src/common/lib/types/protocolmessage.ts
- src/common/lib/client/realtimechannel.ts
- src/plugins/objects/objects.ts
- test/realtime/objects.test.js
- src/plugins/objects/syncobjectsdatapool.ts
- src/plugins/objects/liveobject.ts
- src/plugins/objects/livecounter.ts
- src/plugins/objects/livemap.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.
src/plugins/objects/objectmessage.ts (5)
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#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: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/util/utils.ts:471-472
Timestamp: 2024-11-26T13:09:46.785Z
Learning: In `src/common/lib/util/utils.ts`, avoid suggesting replacing `Function` with `(...args: any[]) => any` in type definitions, as it doesn't improve type safety.
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#2052
File: ably.d.ts:2608-2621
Timestamp: 2025-06-27T08:37:06.539Z
Learning: In ably.d.ts, null values are not supported in LiveMap objects. The PrimitiveObjectValue type correctly excludes null even though JsonScalar includes it, as this reflects the intentional design limitation of the LiveMap implementation.
⏰ 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)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
🔇 Additional comments (7)
src/plugins/objects/objectmessage.ts (7)
194-239: Encoding function implementation looks correct.The
encodefunction properly handles the separation between ObjectMessage and WireObjectMessage formats, with appropriate field-by-field encoding of nested structures.
335-356: Buffer copying limitation is documented and acceptable.Based on retrieved learnings, using
JSON.parse(JSON.stringify(...))for deep copying is acceptable when buffer values are preserved in subsequent code, which appears to be the case here.
374-380: Generic type parameters correctly applied.The ObjectMessage class now properly uses generic ObjectData types for operation and object fields, maintaining type safety.
414-445: Encoding method implementation is sound.The encode method correctly returns a WireObjectMessage and uses appropriate encoding functions for both initial values and object data.
816-819: Return type assertion needs verification.Line 818 uses
as ObjectDatawhich bypasses type checking. Verify this is safe and necessary.The type assertion here converts a potentially failed decode result to ObjectData. Consider if this could lead to runtime issues when the conversion is invalid.
265-310: encodeInitialValue logic verified safeI’ve confirmed that:
- Both LiveMap.createInitialValueObject and LiveCounter.createInitialValueObject return only the payload field (
maporcounter) as a Pick, matching the Partial signature.- ObjectMessage.fromValues supplies default values for all other required properties (action, objectId, etc.) and only uses the provided
initialValuewhen encoding.- encodeInitialValue then performs the two-step “data-level” and “wire-level” encoding exactly as intended.
No changes required.
795-802: Type assertion is appropriate in this context
SinceWireObjectData.bytesis declared asBufferlike | string(string under the JSON protocol), casting toBufferlikewhenformat === 'msgpack'is required to satisfy TypeScript. If you’d like stricter runtime safety, you can add a quick guard:if (format === 'msgpack') { if (typeof objectData.bytes === 'string') { throw new Error('Expected binary data for msgpack'); } decodedBytes = objectData.bytes; } else { // … }but the existing
as Bufferlikedoes not bypass any critical safety checks given the protocol guarantees.
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
5c25961 to
7d2e105
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
7d2e105 to
3a332be
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/plugins/objects/objectmessage.ts (1)
559-600: Error handling allows partial decoding which could cause issuesThis is a continuation of the previous review concern. The decode method catches errors, logs them, but continues execution, potentially returning partially decoded messages with missing or invalid fields.
Consider either throwing the error after logging or adding validation to ensure critical fields were decoded successfully:
} catch (error) { client.Logger.logAction( client.logger, client.Logger.LOG_ERROR, 'WireObjectMessage.decode()', this._utils.inspectError(error), ); + throw new client.ErrorInfo('Failed to decode WireObjectMessage', 92000, 500, error); }
🧹 Nitpick comments (2)
src/plugins/objects/objectmessage.ts (2)
265-310: Review the complex encoding logic in encodeInitialValueThis function has intricate logic that creates an ObjectMessage, encodes it to WireObjectMessage, then encodes for wire transmission. While the comments explain the purpose, consider if this multi-step encoding could be simplified or if additional validation is needed.
Consider adding validation to ensure the encoding steps don't introduce inconsistencies:
// Add validation after encoding if (!initialValueWithDataEncoding) { throw new Error('Failed to encode initial value for wire transmission'); }
608-620: Clever format detection but consider making it explicitThe
toJSONmethod infers the format by checkingarguments.length, which is clever but potentially fragile. Consider making the format detection more explicit for maintainability.Consider adding a comment explaining the format detection logic more clearly:
toJSON() { + // Format detection: JSON.stringify() calls toJSON() with arguments, + // while MSGPack toJSON() is called with empty arguments const format = arguments.length > 0 ? this._utils.Format.json : this._utils.Format.msgpack;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/common/lib/client/realtimechannel.ts(3 hunks)src/common/lib/types/protocolmessage.ts(3 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(11 hunks)src/plugins/objects/livemap.ts(20 hunks)src/plugins/objects/liveobject.ts(4 hunks)src/plugins/objects/objectmessage.ts(20 hunks)src/plugins/objects/objects.ts(2 hunks)src/plugins/objects/syncobjectsdatapool.ts(3 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/plugins/objects/index.ts
- src/common/lib/types/protocolmessage.ts
- src/plugins/objects/syncobjectsdatapool.ts
- test/realtime/objects.test.js
- src/plugins/objects/objects.ts
- src/common/lib/client/realtimechannel.ts
- src/plugins/objects/liveobject.ts
- src/plugins/objects/livecounter.ts
- src/plugins/objects/livemap.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.
src/plugins/objects/objectmessage.ts (7)
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#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: SimonWoolf
PR: ably/ably-js#1923
File: src/common/lib/util/utils.ts:471-472
Timestamp: 2024-11-26T13:09:46.785Z
Learning: In `src/common/lib/util/utils.ts`, avoid suggesting replacing `Function` with `(...args: any[]) => any` in type definitions, as it doesn't improve type safety.
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#2052
File: ably.d.ts:2608-2621
Timestamp: 2025-06-27T08:37:06.539Z
Learning: In ably.d.ts, null values are not supported in LiveMap objects. The PrimitiveObjectValue type correctly excludes null even though JsonScalar includes it, as this reflects the intentional design limitation of the LiveMap implementation.
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#1897
File: src/plugins/liveobjects/liveobjects.ts:145-146
Timestamp: 2024-10-22T14:22:00.033Z
Learning: When reviewing code in the ably/ably-js repository, the user prefers that TODO comments are ignored.
⏰ 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)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (chromium)
🔇 Additional comments (6)
src/plugins/objects/objectmessage.ts (6)
11-11: LGTM: Clear type definition for encoding functionThe updated type signature properly reflects that the encoding function can handle both decoded and wire format data.
48-66: LGTM: Well-designed wire format interfaceThe
WireObjectDatainterface clearly distinguishes fromObjectDataby allowingbytesto be eitherBufferlikeorstring, which properly represents the protocol-specific encoding (binary for msgpack, Base64 string for JSON).
72-76: LGTM: Consistent generic type applicationThe generic type parameters have been consistently applied across all related interfaces (
ObjectsMapOp<TData>,ObjectsMapEntry<TData>,ObjectsMap<TData>,ObjectOperation<TData>,ObjectState<TData>), providing proper type safety for both wire and decoded formats.Also applies to: 92-103, 110-114, 130-148, 169-191
194-239: LGTM: Well-structured encoding functionThe encoding function properly handles all message fields and uses appropriate helper functions for different data types. The approach of deep copying and then encoding specific fields is sound.
338-361: Buffer copying limitation is documented and acceptableBased on the retrieved learning and past discussions, the JSON.parse/stringify approach for deep copying is acceptable here since buffer values are preserved in the original message when needed.
419-449: LGTM: Improved encode method with clear return typeThe updated
encodemethod now properly returns aWireObjectMessageinstead of mutating the original message, which is a significant improvement for immutability and clarity.
Use `WireObjectMessage` to represent the message received from the server which can be decoded into an `ObjectMessage`. `ObjectMessage` can then be encoded into a `WireObjectMessage`.
VeskeR
force-pushed
the
PUB-1672/refactor-objectmessage
branch
from
3a332be to
81580cd
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2051/bundle-report
This should've been the part of [1] but somehow got missed [1] #2037
Use
WireObjectMessageto represent the message received from the server which can be decoded into anObjectMessage.ObjectMessagecan then be encoded into aWireObjectMessage.This matches the style used for messages/presencemessages/annotationmessages which was introduced in #1941 (see Simon's comment to see changes in the PR, as it bugged out and "Files changed" doesn't work for some reason)
Resolves PUB-1672
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Style