Add Ably LiveObjects - Merge integration/liveobjects into main
#2007
Conversation
… Objects Resolves DTP-952
Decoupling between underlying data and client-held reference will be achieved using `_dataRef` property on ancestor LiveObject class. Resolves DTP-953
Resolves DTP-960
Resolves DTP-961
[DTP-947] Expose LiveObjects as a plugin
[DTP-952, DTP-953] Add base implementation for abstract LiveObject, concrete LiveMap/LiveCounter classes
[DTP-960, DTP-961] Add Live Objects access API
…eserializedWithDependencies` This is in preparation for following changes where `fromDeserialized` function would require a LiveObjectsPlugin to create StateMessage classes. We can't include LiveObjectsPlugin in the core library, so this plugin will need to be provided by the tests.
This is in preparation for adding decoding messages to StateMessage. StateMessage has multiple data/encoding entries and requires a more generic function to decode its data, instead of a previous `decode` function that expected data and encoding to be present on a Message class
STATE_SYNC message processing in `RealtimeChannel.processMessage` is based on the process for `PRESENCE` message. Resolves DTP-950
Resolves DTP-949
Plugins can't import EventEmitter class directly as that would increase the bundle size of a plugin
Resolves DTP-951
Co-authored-by: Owen Pearson <[email protected]>
|
Note before merging: UPDATE: removed in 671243c |
Leaf values for objects are now stored in type keys on ObjectData object instead of a single `value` key with a union type. The new approach caused an issue with comet transports (only implemented in ably-js) as comet transports do not support msgpack protocol. We can't rely on the usual `client.options.useBinaryProtocol` option to identify the protocol used, as comet transport changes the selected protocol in its implementation. Instead we get the current active transport object and its format. Resolves PUB-1666
[PUB-1666] Remove `ObjectValue`
github-actions
bot
temporarily deployed
to
staging/pull/2007/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
♻️ Duplicate comments (4)
src/plugins/objects/objectmessage.ts (1)
294-295:⚠️ Potential issue
encodeInitialValue()still firesObjectMessage.encode()without awaiting
ObjectMessage.encode()returns aPromise, but the call remains un-awaited, so the asynchronous encoding chain may still be running while the message is being serialised for the wire. This re-introduces the data-race described in the earlier review (see past comment 283-301).
Consider awaiting the promise (or making the surrounding function sync if possible).-ObjectMessage.encode(msg, client.MessageEncoding); +await ObjectMessage.encode(msg, client.MessageEncoding);src/plugins/objects/livemap.ts (3)
560-614: 🛠️ Refactor suggestion
_updateFromDataDiff()still emits empty updates + unnecessarycontinuesEarlier feedback pointed out that returning
{ update: {} }when nothing changed causes superfluousnotifyUpdated()calls. The logic remains unchanged.
Additionally, thecontinueat line 609 is redundant onceupdate.update[typedKey] = 'updated'has executed.Please:
- Return a
{ noop: true }marker whenupdate.updateis empty.- Drop the extra
continuestatements flagged by Biome.🧰 Tools
🪛 Biome (1.9.4)
[error] 609-609: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
823-837:⚠️ Potential issueNull-guard still missing on
entry.data
entry.datacan beundefinedfor tombstoned entries, yet the code dereferences it unconditionally (entry.data.objectId, etc.).
This will crash the sync loop on a perfectly valid tombstone message.Please apply the earlier guard (see past comment “Missing
datanull-guard …”).
887-903:⚠️ Potential issueEntries pointing to unknown objects should count as tombstoned
_isMapEntryTombstoned()still treats a missing referenced object as not tombstoned, leading to phantom keys inentries()/keys()/values()and an incorrectsize().
The previous review suggested treating both missing and tombstoned referenced objects as tombstoned.- if ('objectId' in data) { - const refObject = this._objects.getPool().get(data.objectId); - - if (refObject?.isTombstoned()) { - return true; - } - } + if ('objectId' in data) { + const refObject = this._objects.getPool().get(data.objectId); + if (!refObject || refObject.isTombstoned()) { + return true; + } + }
🧹 Nitpick comments (1)
src/plugins/objects/objectmessage.ts (1)
483-485:toJSON()protocol detection is fragileRelying on
arguments.lengthto infer the protocol couples the implementation to current (undocumented) behaviour of JSON/MsgPack serializers.
A safer option is to pass theformatexplicitly from the caller or cache the client’suseBinaryProtocolflag on construction.This is a nice-to-have, but it will prevent future regressions if the serialiser implementation changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
ably.d.ts(8 hunks)src/common/lib/client/realtimechannel.ts(12 hunks)src/common/lib/transport/connectionmanager.ts(2 hunks)src/plugins/objects/livemap.ts(1 hunks)src/plugins/objects/objectmessage.ts(1 hunks)test/common/modules/private_api_recorder.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/common/modules/private_api_recorder.js
- src/common/lib/transport/connectionmanager.ts
- src/common/lib/client/realtimechannel.ts
- ably.d.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/plugins/objects/livemap.ts
[error] 609-609: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
See Simon's comment [1]. The `channel` property should not be expected in the wire protocol for object messages, just like it isn't present in a message or presencemessage. [1] ably/specification#279 (comment)
`MapEntry.data` will not be set for tombstone properties in a map, this commit ensures we handle it correctly.
…with HAS_OBJECTS=false Object references must be kept the same whenever possible so that end-user keeps the correct reference and its subscriptions to the object whenever resynchronization happens, see [1]. On channel DETACHED or FAILED only the objects data should be removed, and no update events should be emitted, see [2]. [1] https://ably.atlassian.net/wiki/spaces/LOB/pages/3382738945/LODR-022+Realtime+Client+read-only+internal+spec#7.-Decoupling-of-the-underlying-state-data-and-LiveObject-class-instances [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/3784933378/LODR-032+Realtime+Client+behavior+under+channel+states
…tion is set to false
LiveObjects fixes
VeskeR
changed the title
integration/liveobjects into mainintegration/liveobjects into main
Decided to rename to `AblyObjectsTypes` to avoid using a generic name like `ObjectsTypes` in a global namespace, see [1] [1] ably/ably-js#2007 (comment)
Decided to rename to `AblyObjectsTypes` to avoid using a generic name like `ObjectsTypes` in a global namespace, see [1] [1] ably/ably-js#2007 (comment)
Decided to rename to `AblyObjectsTypes` to avoid using a generic name like `ObjectsTypes` in a global namespace, see [1] [1] ably/ably-js#2007 (comment)
Merge
integration/liveobjectsintomainbranch to add Objects plugin to the library (implementation of LiveObjects feature).All changes were reviewed in the underlying PRs and this PR mainly needs a review from the Ecosystems team regarding the plugin structure, public README and documentation, and library changes in general:
LiveObjects.getRoot()method #1891StateObject#1924ably.d.tstypes) #1928siteCodefield in StateMessages #1926mainintointegration/liveobjects2024-11-29 #1930_decodeAndPrepareMessagesfor processing ofSTATEandSTATE_SYNCmessages #1931StateObject.tombstoneandOBJECT_DELETEmessages #1934channel.properties.channelSerialwhen receivingSTATEmessages #1961ConnectionDetails.maxMessageSizelimit to state messages #1963MsgPackandJSONprotocols, and on all transports #1965ably.d.tsfor LiveObjects #1967LiveMapandBatchContextLiveMap#1981HAS_STATEflag #1985amountproperty instead ofinc#1991@experimentaltags to LiveObjects methods #1996deep-equalwithdequalpackage #2011mainintointegration/liveobjectsbranch 2025-04-16 #2006mainintointegration/liveobjectsbranch 2025-04-17 #2014ObjectValue#2026Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores