Remove non-path-based API from LiveObjects #2108
Conversation
WalkthroughRemoved legacy lifecycle APIs and deprecated Live* types, replaced SubscribeResponse with Subscription/StatusSubscription, generalized LiveMap types to Record<string, API.Value>, changed RealtimeObject.get() to return PathObject<LiveMap>, and introduced global AblyObjectsTypes plus JSON type aliases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealtimeObject
participant DefaultPathObject
participant LiveMap
rect rgba(200,220,255,0.6)
Note over Client,RealtimeObject: New control flow: get() → PathObject<LiveMap>
end
Client->>RealtimeObject: get()
RealtimeObject->>LiveMap: ensure root LiveMap is available (sync wait)
RealtimeObject->>DefaultPathObject: wrap LiveMap in DefaultPathObject
DefaultPathObject-->>Client: return PathObject<LiveMap<T>>
Client->>DefaultPathObject: subscribe(listener)
DefaultPathObject-->>Client: return Subscription (unsubscribe)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
github-actions
bot
temporarily deployed
to
staging/pull/2108/features
github-actions
bot
temporarily deployed
to
staging/pull/2108/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2108/typedoc
![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
📜 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 (13)
ably.d.ts(7 hunks)objects.d.ts(1 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livemap.ts(11 hunks)src/plugins/objects/livemapvaluetype.ts(2 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectmessage.ts(1 hunks)src/plugins/objects/objectspool.ts(1 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(2 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)test/common/modules/private_api_recorder.js(0 hunks)
💤 Files with no reviewable changes (1)
- test/common/modules/private_api_recorder.js
🧰 Additional context used
🧬 Code graph analysis (11)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (1)
Primitive(2373-2381)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (3)
LiveMap(2415-2418)Subscription(1680-1689)Value(2438-2438)src/plugins/objects/livemap.ts (1)
LiveMap(48-983)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
ably.d.ts (1)
Subscription(1680-1689)
src/plugins/objects/batchcontext.ts (1)
src/plugins/objects/livemap.ts (1)
LiveMap(48-983)
src/plugins/objects/liveobject.ts (2)
ably.d.ts (2)
EventCallback(1653-1653)Subscription(1680-1689)src/plugins/objects/instance.ts (1)
InstanceEvent(22-25)
src/plugins/objects/objectspool.ts (3)
ably.d.ts (1)
LiveMap(2415-2418)objects.d.ts (1)
LiveMap(19-31)src/plugins/objects/livemap.ts (1)
LiveMap(48-983)
src/plugins/objects/realtimeobject.ts (1)
ably.d.ts (3)
Value(2438-2438)AblyDefaultObject(2487-3539)LiveMap(2415-2418)
src/plugins/objects/instance.ts (1)
ably.d.ts (4)
EventCallback(1653-1653)InstanceSubscriptionEvent(3688-3693)Subscription(1680-1689)LiveObject(2431-2431)
src/plugins/objects/livemapvaluetype.ts (2)
src/plugins/objects/livemap.ts (2)
LiveMap(48-983)ValueObjectData(24-27)ably.d.ts (2)
LiveMap(2415-2418)Primitive(2373-2381)
src/plugins/objects/livemap.ts (4)
ably.d.ts (6)
Primitive(2373-2381)Value(2438-2438)LiveMap(2415-2418)LiveObject(2431-2431)ObjectMessage(3754-3801)CompactedValue(2444-2469)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(18-26)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(38-489)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(358-462)
ably.d.ts (4)
objects.d.ts (1)
LiveMap(19-31)src/plugins/objects/livemap.ts (1)
LiveMap(48-983)src/plugins/objects/realtimeobject.ts (1)
ObjectsEventCallback(32-32)test/package/browser/template/src/index-objects.ts (1)
AblyObjectsTypes(27-29)
🪛 GitHub Actions: Test NPM package
objects.d.ts
[error] 14-14: Cannot redeclare exported variable 'LiveMap'.
⏰ 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). (6)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (11)
src/plugins/objects/batchcontext.ts (1)
84-84: LGTM! Correct alignment with function signature.The removal of the cast to
Primitiveis correct, asLiveMap.createMapSetMessageexpectsvalue: API.Value, which can be a primitive,LiveCounterValueType, orLiveMapValueType.src/plugins/objects/pathobjectsubscriptionregister.ts (1)
6-6: LGTM! Consistent with API refactoring.The change from
SubscribeResponsetoSubscriptionaligns with the PR-wide transition to Subscription-based abstractions. The implementation correctly returns an object with anunsubscribemethod matching theSubscriptioninterface.Also applies to: 63-63
src/plugins/objects/objectmessage.ts (1)
44-44: LGTM! Type standardization.The change from
PrimitiveObjectValuetoAPI.Primitivestandardizes primitive value typing across the codebase and aligns with the removal of thePrimitiveObjectValuealias.src/plugins/objects/livemapvaluetype.ts (1)
77-77: LGTM! Consistent type handling.Both changes correctly align with the removal of
PrimitiveObjectValueand the standardization onAPI.Primitive. Line 77 removes an unnecessary cast sinceLiveMap.validateKeyValueexpectsvalue: API.Value, and line 141 correctly types primitive values asAPI.PrimitiveforValueObjectData.Also applies to: 141-141
src/plugins/objects/objectspool.ts (1)
33-34: LGTM! API simplification.The removal of the generic type parameter simplifies the API and aligns with the broader refactoring to use non-generic
LiveMap. This change is consistent with the PathObject-based access pattern introduced in the PR.src/plugins/objects/pathobject.ts (1)
14-14: LGTM! Consistent API refactoring.All changes align with the PR objectives:
- Lines 14, 354: Migration from
SubscribeResponsetoSubscriptionaligns with the new subscription-based abstractions.- Line 34: Removing the generic type parameter from
LiveMapsimplifies the internal API.- Line 385: Direct assignment is cleaner since
this._rootis alreadyLiveMap, which extendsValue.These changes are consistent with the broader transition to PathObject-based access patterns.
Also applies to: 34-34, 354-354, 385-385
src/plugins/objects/instance.ts (1)
12-12: LGTM! Proper subscription delegation.The changes correctly implement the migration to
Subscription:
- Lines 12, 172: Import and return type updated to use
Subscription.- Line 176: Implementation properly delegates to the underlying
LiveObject'ssubscribemethod and transforms the internalInstanceEventto the publicInstanceSubscriptionEventformat, including convertingObjectMessageto its user-facing representation.Also applies to: 172-172, 176-176
src/plugins/objects/realtimeobject.ts (1)
82-91: Return type upgrade looks solid.Gating on the existing sync semaphore before handing back the
DefaultPathObjectkeeps the old lifecycle guarantees while exposing the new path-based surface. Nicely done.src/plugins/objects/liveobject.ts (1)
71-81: Subscription wrapper matches the new contract.Switching to the
{ unsubscribe }shape plugs straight into the newSubscriptioninterface without altering the underlying emitter behaviour. Looks good to me.src/plugins/objects/livemap.ts (1)
499-525: compact still normalizes nested structures correctly.Iterating via
entries()means we continue to skip tombstoned keys and base64 wrap buffers, so the observable JSON footprint stays intact after the generics refactor.ably.d.ts (1)
1670-1710: Typings line up with the runtime changes.The new
Subscription/StatusSubscriptioninterfaces give consumers a consistentunsubscribe/offcontract and match the objects returned in code. Looks good.
VeskeR
force-pushed
the
PUB-2065/path-based-batch-api
branch
2 times, most recently
from
aea9de3 to
80fd55e
Compare
VeskeR
force-pushed
the
liveobjects/remove-non-path-based-api
branch
2 times, most recently
from
81e9e97 to
e7d734e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2108/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2108/features
github-actions
bot
temporarily deployed
to
staging/pull/2108/typedoc
mschristensen
left a comment
mschristensen
left a comment
There was a problem hiding this comment.
Small issue to fix on the test otherwise looks great
test/realtime/objects.test.js
Outdated
| const entryInstance = entryPathObject.instance(); | ||
|
|
||
| await Promise.all( | ||
| expectedKeys.map((key) => (entryInstance.get(key) ? undefined : waitForMapKeyUpdate(entryInstance, key))), | ||
| ); | ||
| } | ||
|
|
||
| describe('realtime/objects', function () { | ||
| describe.only('realtime/objects', function () { |
There was a problem hiding this comment.
We should remove this .only
There was a problem hiding this comment.
| ? T | ||
| : any; | ||
|
|
||
| declare global { |
There was a problem hiding this comment.
Unrelated to this PR specifically, but:
I had a thought about this default typings declaration. I don't know this this is very useful to be honest, since you likely use multiple channels in your app, and each channel probably needs a different schema. Therefore I am tempted to remove this in the new version of the API and rely only on the type parameter approach.
Let's discuss
There was a problem hiding this comment.
Seeing in our demos how easy it is to provide an explicit type parameter the first time you call object.get<MyObject> I'd agree, seems like there is no actual need to have a global interface.
Also explaning how to use global type is harder than just saying "provide type for your object as a type parameter to the .get() call", so it's a win-win
VeskeR
force-pushed
the
PUB-2065/path-based-batch-api
branch
from
80fd55e to
58bcf67
Compare
VeskeR
force-pushed
the
liveobjects/remove-non-path-based-api
branch
from
e7d734e to
70540ff
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2108/features
github-actions
bot
temporarily deployed
to
staging/pull/2108/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2108/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/plugins/objects/liveobject.ts (1)
3-3: LiveObject.subscribe and path notifications match the new subscription modelReturning a
{ unsubscribe }Subscriptionfromsubscribe()is consistent with the new public typings and integrates cleanly withDefaultInstance.subscribe.notifyUpdatedstill clears all listeners on tombstone, so subscriptions remain self-cleaning on delete.In
_notifyPathSubscriptions, usingObject.keys(update.update)under the'LiveMapUpdate'guard is fine as long as map updates always provide a concreteupdateobject rather thanundefined. If there is any path where a non-noopLiveMapUpdatecould haveupdateabsent, adding a defensiveif (update.update)guard would avoid a possibleObject.keys(undefined)at runtime.Also applies to: 71-81, 307-319
ably.d.ts (2)
1669-1712: Subscription / StatusSubscription wiring looks coherent across the APIThe new
SubscriptionandStatusSubscriptioninterfaces, and their use in:
RealtimeObject.get(returningPromise<PathObject<LiveMap<T>>>),RealtimeObject.on(returningStatusSubscription),PathObjectBase.subscribe(returningSubscription),InstanceBase.subscribe(returningSubscription),all line up with the implementations in
liveobject.ts,instance.ts,pathobject.ts, andrealtimeobject.ts, which return simple objects exposingunsubscribeoroff. The structural types match the runtime shapes, so this should be a non-breaking, clearer replacement for the oldSubscribeResponse.Only minor nit: you now have both
OnObjectsEventResponseinrealtimeobject.tsandStatusSubscriptionhere describing effectively the same shape. At some point it may be worth consolidating those to avoid parallel concepts, but that's optional.Also applies to: 2315-2351, 2497-2533, 3383-3413
2373-2402: Json aliases are sensible; confirm JsonObject’s allowance forundefinedis intentional*Adding
Json,JsonScalar,JsonArray, andJsonObjectand then usingJsonArray | JsonObjectinsidePrimitivemakes the value-space for LiveObjects explicit and JSON-oriented, which is helpful.
JsonObjectis defined as{ [prop: string]: Json | undefined }, which allows properties whose value type includesundefined. That’s reasonable if you want to model optional fields in user data, but it technically goes beyond strict JSON (whereundefinedvalues are typically omitted rather than represented). If your intent is to mirror “JSON-encodable data with optional properties” rather than strict JSON, this is fine; otherwise, consider tightening this toJsonand letting optional keys model absence.
📜 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 (12)
ably.d.ts(7 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livemap.ts(11 hunks)src/plugins/objects/livemapvaluetype.ts(2 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectmessage.ts(1 hunks)src/plugins/objects/objectspool.ts(1 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(2 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)test/common/modules/private_api_recorder.js(0 hunks)
💤 Files with no reviewable changes (1)
- test/common/modules/private_api_recorder.js
🚧 Files skipped from review as they are similar to previous changes (5)
- src/plugins/objects/objectmessage.ts
- src/plugins/objects/objectspool.ts
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/pathobject.ts
- src/plugins/objects/batchcontext.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/instance.ts (1)
ably.d.ts (4)
EventCallback(1653-1653)InstanceSubscriptionEvent(3688-3693)Subscription(1680-1689)LiveObject(2431-2431)
src/plugins/objects/realtimeobject.ts (2)
ably.d.ts (3)
Value(2438-2438)AblyDefaultObject(2487-3539)LiveMap(2415-2418)src/plugins/objects/livemap.ts (1)
LiveMap(48-983)
src/plugins/objects/livemapvaluetype.ts (2)
src/plugins/objects/livemap.ts (2)
LiveMap(48-983)ValueObjectData(24-27)ably.d.ts (2)
LiveMap(2415-2418)Primitive(2373-2381)
src/plugins/objects/liveobject.ts (2)
ably.d.ts (2)
EventCallback(1653-1653)Subscription(1680-1689)src/plugins/objects/instance.ts (1)
InstanceEvent(22-25)
ably.d.ts (4)
src/plugins/objects/livemap.ts (1)
LiveMap(48-983)objects.d.ts (1)
LiveMap(16-28)src/plugins/objects/realtimeobject.ts (1)
ObjectsEventCallback(32-32)test/package/browser/template/src/index-objects.ts (1)
AblyObjectsTypes(27-29)
src/plugins/objects/livemap.ts (2)
ably.d.ts (6)
Primitive(2373-2381)Value(2438-2438)LiveMap(2415-2418)LiveObject(2431-2431)ObjectMessage(3754-3801)CompactedValue(2444-2469)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(18-26)
⏰ 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). (6)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (10)
src/plugins/objects/livemapvaluetype.ts (1)
77-77: Initial entry validation and primitive casting look correctUsing
LiveMap.validateKeyValueforentriesensures initial map state is validated consistently with runtimesetoperations, and casting leaf values toAPI.PrimitiveinValueObjectDataaligns with the newObjectData.valuetyping. I don't see behavioural regressions here.Also applies to: 141-143
src/plugins/objects/instance.ts (1)
12-14: Instance.subscribe now aligned with Subscription APIImporting
Subscriptionand returning it fromDefaultInstance.subscribematches the new public typings, andsubscribeIteratorcorrectly reuses theunsubscribefunction from that object. The runtime behaviour stays the same while the API surface becomes consistent.Also applies to: 172-182
src/plugins/objects/realtimeobject.ts (1)
82-92: get() returning a PathObject wrapper is consistent with the path-based APISwitching
get()to return aDefaultPathObjectaround the rootLiveMap(instead of the map itself) matches the newPathObject<LiveMap<T>>return type while preserving the existing sync-wait semantics. This is the right place to centralize the path-based API.ably.d.ts (1)
2471-2494: AblyObjectsTypes / AblyDefaultObject typing pattern looks correctThe global
AblyObjectsTypeswith an index signature plus theAblyDefaultObjectconditional:
- Defaults to
Record<string, Value>when users don’t provide a customAblyObjectsTypes['object'].- Switches to the user-provided
AblyObjectsTypes['object']when present and constrained toRecord<string, Value>.- Produces a clear string literal error type if the user’s
objecttype doesn’t match the expected shape.That matches the documentation comments and the example given in the
RealtimeObject.getdoc, and is a neat way to let advanced users strongly type their root object without impacting default ergonomics. I’d just suggest double-checking with your example app / template that augmentingAblyObjectsTypesbehaves as expected under your supported TypeScript versions.src/plugins/objects/livemap.ts (6)
24-52: LGTM! Type generalization aligns with path-based API migration.The changes successfully generalize the LiveMap type system:
ValueObjectData.valuecorrectly usesAPI.Primitivematching the type definitionLiveMapUpdate<T>andLiveMap<T>now useRecord<string, API.Value>as the type constraint with proper defaults- The branded interface pattern with
[API.__livetype]: 'LiveMap'provides type-safe discrimination at compile timeThese changes are consistent with the broader migration to value-centric typing.
68-82: LGTM! Static factory methods correctly use non-generic return types.The removal of generic type parameters from
zeroValueandfromObjectStateis consistent with the type generalization strategy, whereLiveMapnow defaults toRecord<string, API.Value>.
196-218: LGTM! Value resolution correctly handles API.Value types.The
getmethod and_getResolvedValueFromObjectDataproperly handle the value-centric model:
- Tombstoned objects and missing entries return
undefinedas expected- Line 922 safely casts the internal
LiveObjectto the publicAPI.LiveObjectinterface- Return types align with the
API.Valuetype definitionAlso applies to: 903-923
499-525: LGTM! Compact method correctly implements recursive value compaction.The return type
API.CompactedValue<API.LiveMap<T>>properly leverages the type-level transformation that:
- Recursively compacts nested
LiveMapvalues to plain objects- Converts
LiveCountertonumber- Transforms
Buffer/ArrayBufferto base64 strings- Preserves other primitives as-is
The implementation correctly handles all these cases.
141-161: LGTM! Method signatures are consistent with type system changes.All updated method signatures correctly use:
stringkeys throughout (no generic key types)API.Valuefor values- Proper generic constraints with
TKey extends keyof T & stringin iterators- Correct handling of
ObjectIdObjectDatavsValueObjectDatafor parent reference trackingAlso applies to: 236-294, 477-490, 947-982
87-182: Review comment is valid but reflects design limitation, not a bug.The type safety concern is real:
validateKeyValueaccepts any object (line 173-181) while the cast toAPI.Primitiveexpects only specific types (string, number, boolean, Buffer, ArrayBuffer, JsonArray, JsonObject).However, verification shows this is handled by design: the encodeObjectDataFn at line 412-427 uses JSON.stringify for objects, which silently drops non-serializable properties. This means objects with methods or symbols will lose data during serialization, but without throwing errors.
The validation is intentionally loose to provide flexibility, with serialization as the final safety gate. If this behavior is acceptable for your use case, no changes are needed. If you want stricter type checking to prevent silent data loss,
validateKeyValuecould be enhanced to reject objects with non-serializable properties.
This marks full transition to path-based API for LiveObjects. Included in this PR: - resolve TODOs related to path-based API - update all LiveObjects tests to use path-based API - remove publicly exposed LiveObject, LiveMap and LiveCounter types from ably.d.ts - remove lifecycle subscriptions API for liveobjects (API and implementation). OBJECT_DELETE events are handled by regular subscription events. - update package tests to use the new path-based API for Objects
VeskeR
force-pushed
the
liveobjects/remove-non-path-based-api
branch
from
70540ff to
9c9d510
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/package/browser/template/src/index-objects.ts (1)
2-24: Fix LiveMapPathObject generic argument to use the map entry type, not the LiveMap wrapperThe examples overall are aligned with the new path-based API, but this line is likely wrong:
const map: Ably.LiveMapPathObject<MyCustomObject['mapKey']> = myObject.get('mapKey');
MyCustomObject['mapKey']isLiveMap<{ foo: 'bar'; nestedMap?: LiveMap<{ baz: 'qux' }> }>LiveMapPathObject<T>expectsT extends Record<string, Ably.Value>describing the map’s entry shape, not theLiveMap<...>wrapper type.This will typically violate the generic constraint and can also break
get('foo')key typing.You probably want the entry type instead, e.g.:
type MapEntryType = MyCustomObject['mapKey'] extends LiveMap<infer U> ? U : never; const map: Ably.LiveMapPathObject<MapEntryType> = myObject.get('mapKey');The rest of the typings in this file (
PathObject<LiveMap<MyCustomObject>>,.get(...).value(), subscription callback types, and the explicit-objectget<ExplicitObjectType>()) look consistent with the new declarations.Also applies to: 44-63, 67-77
src/plugins/objects/livemap.ts (2)
903-923: LiveCounter is missingimplements API.LiveCounterdeclaration.The double cast at line 922 exists because
LiveCounter(line 15 in src/plugins/objects/livecounter.ts) extendsLiveObjectbut does not declareimplements API.LiveCounter, unlikeLiveMapwhich properly declaresimplements API.LiveMap<T>.The cast
refObject as unknown as API.LiveObjectis a workaround for this missing interface implementation. To eliminate the unsafe cast and improve type safety, add the interface declaration:export class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate> implements API.LiveCounter {This would align
LiveCounterwithLiveMap's pattern and allow proper type narrowing without requiring the double cast.
87-118: Type safety issue:createMapSetMessage()cannot handle bareLiveObjectinstances despite accepting them in the type signature.The method accepts
value: API.Value(which includesLiveMap | LiveCounterinstances), but the implementation only checks forLiveCounterValueType.instanceof()andLiveMapValueType.instanceof(). If a bareLiveMaporLiveCounterinstance is passed—which is allowed by the type system throughDefaultBatchContext.set(key: string, value: Value)at line 77 ofbatchcontext.ts—the code falls through to line 116 and incorrectly casts it toAPI.Primitive.Either narrow the parameter type to exclude bare
LiveObjectinstances, or add runtime validation to reject them with a clear error message.
🧹 Nitpick comments (2)
src/plugins/objects/realtimeobject.ts (1)
34-36: Internal OnObjectsEventResponse vs public StatusSubscription naming
RealtimeObject.onstill returns an internalOnObjectsEventResponse({ off(): void }), while the public typings now exposeStatusSubscription. Structurally these match, so there is no runtime or typing bug, but you may want to:
- Rename
OnObjectsEventResponsetoStatusSubscription(or import the public type) for consistency and to avoid future drift.Also applies to: 94-103
src/plugins/objects/liveobject.ts (1)
307-319: Consider guarding LiveMapUpdate.updatedKeys computation against malformed updatesHere:
if (update._type === 'LiveMapUpdate') { const updatedKeys = Object.keys(update.update); for (const key of updatedKeys) { // ... } }this assumes
update.updateis always a non-null object. If a malformedLiveMapUpdatewere ever constructed withupdate.updatenullor non-object, this would throw.Given this is internal, it’s low risk, but you could defensively narrow for robustness:
if (update._type === 'LiveMapUpdate' && update.update && typeof update.update === 'object') { const updatedKeys = Object.keys(update.update as Record<string, unknown>); // ... }
📜 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 (13)
ably.d.ts(7 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livemap.ts(11 hunks)src/plugins/objects/livemapvaluetype.ts(2 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectmessage.ts(1 hunks)src/plugins/objects/objectspool.ts(1 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(2 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)test/common/modules/private_api_recorder.js(0 hunks)test/package/browser/template/src/index-objects.ts(3 hunks)
💤 Files with no reviewable changes (1)
- test/common/modules/private_api_recorder.js
🚧 Files skipped from review as they are similar to previous changes (6)
- src/plugins/objects/batchcontext.ts
- src/plugins/objects/objectmessage.ts
- src/plugins/objects/objectspool.ts
- src/plugins/objects/pathobject.ts
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/livemapvaluetype.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/instance.ts (1)
ably.d.ts (4)
EventCallback(1653-1653)InstanceSubscriptionEvent(3688-3693)Subscription(1680-1689)LiveObject(2431-2431)
src/plugins/objects/realtimeobject.ts (2)
ably.d.ts (3)
Value(2438-2438)AblyDefaultObject(2487-3539)LiveMap(2415-2418)src/plugins/objects/livemap.ts (1)
LiveMap(48-983)
src/plugins/objects/liveobject.ts (2)
ably.d.ts (2)
EventCallback(1653-1653)Subscription(1680-1689)src/plugins/objects/instance.ts (1)
InstanceEvent(22-25)
test/package/browser/template/src/index-objects.ts (4)
ably.d.ts (2)
LiveMap(2415-2418)LiveCounter(2423-2426)src/plugins/objects/batchcontext.ts (2)
size(71-75)value(30-34)src/plugins/objects/instance.ts (2)
size(133-139)value(76-104)src/plugins/objects/pathobject.ts (2)
size(259-276)value(144-183)
ably.d.ts (4)
src/plugins/objects/livemap.ts (1)
LiveMap(48-983)objects.d.ts (1)
LiveMap(16-28)src/plugins/objects/realtimeobject.ts (1)
ObjectsEventCallback(32-32)test/package/browser/template/src/index-objects.ts (1)
AblyObjectsTypes(27-29)
src/plugins/objects/livemap.ts (3)
ably.d.ts (6)
Primitive(2373-2381)Value(2438-2438)LiveMap(2415-2418)LiveObject(2431-2431)ObjectMessage(3754-3801)CompactedValue(2444-2469)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(18-26)objects.d.ts (1)
LiveMap(16-28)
⏰ 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). (6)
- 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-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (14)
ably.d.ts (6)
1669-1711: Subscription / StatusSubscription abstractions look consistentThe new
SubscriptionandStatusSubscriptioninterfaces are minimal and align with howsubscribe/onare implemented internally (returning objects withunsubscribe/offmethods). No issues from a typing or runtime perspective.
2315-2341: RealtimeObject.get typing matches the new path-based API
get<T extends Record<string, Value> = AblyDefaultObject>(): Promise<PathObject<LiveMap<T>>>nicely ties together:
- Defaulting
TviaAblyDefaultObjectfor global configuration.- Returning a
PathObject<LiveMap<T>>instead of a rawLiveMap, consistent with the rest of the path-based surface.This matches the implementation in
src/plugins/objects/realtimeobject.tsand provides a clear, type-safe entrypoint.
2383-2402: Json / Primitive type refinements are reasonableIntroducing
Json,JsonScalar,JsonArray, andJsonObject, and includingJsonArray/JsonObjectinPrimitive, cleanly expresses “JSON-encodable” values and remains compatible with the existing runtime checks that treat plain objects and arrays as primitives. No concerns here.
2471-2494: AblyObjectsTypes / AblyDefaultObject conditional typing is soundThe
declare global AblyObjectsTypeswith an index signature plus:export type AblyDefaultObject = unknown extends AblyObjectsTypes['object'] ? Record<string, Value> : AblyObjectsTypes['object'] extends Record<string, Value> ? AblyObjectsTypes['object'] : `Provided type definition for the channel \`object\` in AblyObjectsTypes is not of an expected Record<string, Value> type`;gives:
- A sensible default (
Record<string, Value>) when users do not augmentAblyObjectsTypes.- A strict check that user-provided
objecttypes areRecord<string, Value>, with a readable error string otherwise.This interacts correctly with
RealtimeObject.get’s default generic and is a good pattern for global customization.
2527-2533: PathObjectBase.subscribe returning Subscription is consistentChanging
PathObjectBase.subscribeto returnSubscriptionaligns the public typings with the internal pattern of returning{ unsubscribe }fromDefaultPathObject.subscribe, and with the new reusableSubscriptionabstraction. No issues.
3410-3413: InstanceBase.subscribe using Subscription matches implementation
InstanceBase.subscribe(listener: EventCallback<InstanceSubscriptionEvent<T>>): SubscriptionmatchesDefaultInstance.subscribeinsrc/plugins/objects/instance.ts, which returns an object with anunsubscribemethod. This unifies the subscription model across path and instance APIs.src/plugins/objects/instance.ts (1)
12-14: Instance subscriptions correctly adopt the new Subscription typeImporting
Subscriptionand updating:
subscribe(listener): Subscriptionto wrap the underlyingLiveObject.subscribe, andsubscribeIterator()to use the returned{ unsubscribe }keeps behavior the same while aligning with the new public
Subscriptionabstraction. Type and runtime wiring look correct.Also applies to: 172-192
src/plugins/objects/realtimeobject.ts (1)
82-92: RealtimeObject.get now correctly returns a root PathObjectThe new
get<T>():
- Validates channel configuration, waits for initial sync if necessary, and
- Returns
new DefaultPathObject(this, this._objectsPool.getRoot(), [])which is consistent with the public type
Promise<API.PathObject<API.LiveMap<T>>>and the path-based API design. The sync-wait semantics are preserved; only the return type has shifted to the new PathObject abstraction.src/plugins/objects/liveobject.ts (1)
3-4: LiveObject.subscribe correctly adopts Subscription and remains aligned with access checksImporting
Subscriptionand implementing:subscribe(listener: EventCallback<InstanceEvent>): Subscription { this._realtimeObject.throwIfInvalidAccessApiConfiguration(); this._subscriptions.on(LiveObjectSubscriptionEvent.updated, listener); const unsubscribe = () => this._subscriptions.off(LiveObjectSubscriptionEvent.updated, listener); return { unsubscribe }; }is consistent with the new public API and with how
DefaultInstance.subscribeconsumes it. Access validation and cleanup behavior are correct.Also applies to: 71-81
src/plugins/objects/livemap.ts (5)
26-26: LGTM - Type changes align with path-based API transition.The changes to use
API.Primitivefor value storage, widenLiveMaptoRecord<string, API.Value>, and add the branded type marker[API.__livetype]: 'LiveMap'are consistent with the transition to a path-based, API-centric model described in the PR objectives.Also applies to: 42-52
68-82: Static constructors now return non-genericLiveMap.The removal of generic type parameters from
zeroValueandfromObjectStatereduces type information at call sites but aligns with the transition to a simpler path-based API. This trade-off appears intentional.
166-182: LGTM - Validation updated for string keys and API.Value.The validation correctly checks that keys are strings and values are of supported types. The check at lines 173-179 permits
string,number,boolean, andobjecttypes, which aligns withAPI.Primitive(includingBuffer,ArrayBuffer,JsonArray,JsonObjectas objects).
197-218: LGTM - Simplified return logic for undefined cases.The changes to return
undefineddirectly for tombstoned map, missing entries, and tombstoned entries (lines 201, 208, 213) simplify the code and are consistent with the updated type model.
499-525: LGTM - Updated return type aligns with API-centric model.The return type
API.CompactedValue<API.LiveMap<T>>correctly represents the recursive unwrapping behavior whereLiveMapbecomes a plain object,LiveCounterbecomes a number, and binary types become base64 strings. The implementation is consistent with this type.
VeskeR
merged commit 882064f
into
integration/objects-breaking-api
This marks full transition to path-based API for LiveObjects. Included in this PR:
Summary by CodeRabbit
New Features
Refactor
Chores