[PUB-2065] Update LiveObjects batch API to use PathObject/Instance and handle object creation #2107
Conversation
WalkthroughReplaces the legacy batch callback API with a typed BatchFunction/BatchContext system. Adds DefaultBatchContext and RootBatchContext, moves per-type wrappers into instance-backed contexts, updates LiveMap to emit async multi-message sets, adds Instance/PathObject.batch, and removes the RealtimeObject root batch API. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Instance/PathObject
participant RootBatchContext
participant DefaultBatchContext
participant RealtimeObject
User->>Instance/PathObject: batch(fn: BatchFunction)
activate Instance/PathObject
Instance/PathObject->>RootBatchContext: create RootBatchContext
activate RootBatchContext
RootBatchContext->>DefaultBatchContext: provide BatchContext for instance
DefaultBatchContext->>RootBatchContext: queueMessages(msgCtor)
Note right of RootBatchContext `#DDFFDD`: msg constructors stored
Instance/PathObject->>RootBatchContext: await flush()
RootBatchContext->>RootBatchContext: execute msgCtors -> messages[]
RootBatchContext->>RealtimeObject: publish(messages[])
RootBatchContext->>Instance/PathObject: close()
deactivate RootBatchContext
deactivate Instance/PathObject
User-->>User: Promise resolves
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.39.7)test/realtime/objects.test.jsThanks 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/2107/features
github-actions
bot
temporarily deployed
to
staging/pull/2107/bundle-report
VeskeR
force-pushed
the
PUB-2065/path-based-batch-api
branch
from
5019f75 to
8b57e60
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2107/features
github-actions
bot
temporarily deployed
to
staging/pull/2107/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
📜 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 (10)
ably.d.ts(9 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/batchcontextlivecounter.ts(0 hunks)src/plugins/objects/batchcontextlivemap.ts(0 hunks)src/plugins/objects/instance.ts(4 hunks)src/plugins/objects/livemap.ts(3 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/realtimeobject.ts(0 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)
💤 Files with no reviewable changes (3)
- src/plugins/objects/batchcontextlivemap.ts
- src/plugins/objects/realtimeobject.ts
- src/plugins/objects/batchcontextlivecounter.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/pathobject.ts (3)
ably.d.ts (3)
LiveMap(2377-2380)BatchFunction(1672-1672)BatchContext(3004-3010)src/plugins/objects/objectid.ts (1)
LiveObjectType(4-4)src/plugins/objects/rootbatchcontext.ts (1)
RootBatchContext(11-78)
src/plugins/objects/livemap.ts (4)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(38-502)src/plugins/objects/livecountervaluetype.ts (1)
LiveCounterValueType(20-97)src/plugins/objects/livemapvaluetype.ts (1)
LiveMapValueType(34-166)src/plugins/objects/objectmessage.ts (2)
ObjectMessage(360-464)PrimitiveObjectValue(36-36)
src/plugins/objects/rootbatchcontext.ts (3)
src/plugins/objects/batchcontext.ts (1)
DefaultBatchContext(15-118)ably.d.ts (3)
ObjectMessage(3614-3661)Instance(3512-3518)Value(2400-2400)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(38-502)
src/plugins/objects/instance.ts (2)
ably.d.ts (5)
LiveMap(2377-2380)BatchFunction(1672-1672)LiveObject(2393-2393)BatchContext(3004-3010)LiveCounter(2385-2388)src/plugins/objects/rootbatchcontext.ts (1)
RootBatchContext(11-78)
src/plugins/objects/batchcontext.ts (2)
src/plugins/objects/instance.ts (3)
InstanceEvent(22-25)value(76-104)DefaultInstance(27-217)src/plugins/objects/rootbatchcontext.ts (2)
InstanceEvent(6-9)RootBatchContext(11-78)
ably.d.ts (6)
src/plugins/objects/livemap.ts (1)
LiveMap(49-994)objects.d.ts (2)
LiveMap(16-28)LiveCounter(33-43)src/plugins/objects/batchcontext.ts (1)
value(35-38)src/plugins/objects/instance.ts (1)
value(76-104)src/plugins/objects/pathobject.ts (1)
value(145-184)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] 1-1: TypeDoc warnings: several members (e.g., LiveMapBatchContext.set, .set.key, .set.value, remove.__type, and other __type members) do not have documentation in ably.d.ts.
⏰ 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 (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
| if (this._queuedMessageConstructors.length > 0) { | ||
| await this._realtimeObject.publish(msgs); | ||
| } | ||
| } finally { |
There was a problem hiding this comment.
Guard against empty publishes before flushing
If any queued constructor resolves to an empty ObjectMessage array, we still call publish([]) because the guard only checks _queuedMessageConstructors.length. RealtimeObject.publish reduces the array without an initial value, so an empty publish throws at runtime. Please gate on msgs.length > 0 (or otherwise skip the publish) to avoid a hard failure.
- if (this._queuedMessageConstructors.length > 0) {
+ if (msgs.length > 0) {
await this._realtimeObject.publish(msgs);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this._queuedMessageConstructors.length > 0) { | |
| await this._realtimeObject.publish(msgs); | |
| } | |
| } finally { | |
| if (msgs.length > 0) { | |
| await this._realtimeObject.publish(msgs); | |
| } | |
| } finally { |
🤖 Prompt for AI Agents
In src/plugins/objects/rootbatchcontext.ts around lines 38 to 41, the code only
checks this._queuedMessageConstructors.length before calling await
this._realtimeObject.publish(msgs), which can still call publish([]) if
constructors resolve to empty arrays; change the guard to skip publishing when
msgs is empty by checking msgs.length > 0 (or otherwise return/continue) before
calling publish so publish never receives an empty array.
…ject creation This also removes the now-obsolete RealtimeObject.batch() API. Resolves PUB-2065
VeskeR
force-pushed
the
PUB-2065/path-based-batch-api
branch
from
8b57e60 to
1244065
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/plugins/objects/rootbatchcontext.ts (1)
36-40: Guard: skip publish([]) and publish only when msgs.length > 0Current check can still call publish([]) when ctors resolve to empty arrays. Switch the guard to msgs.length.
- if (this._queuedMessageConstructors.length > 0) { - await this._realtimeObject.publish(msgs); - } + if (msgs.length > 0) { + await this._realtimeObject.publish(msgs); + }
🧹 Nitpick comments (11)
scripts/moduleReport.ts (1)
344-346: Allowlist updated for RootBatchContext — good.Nice catch adding
src/plugins/objects/rootbatchcontext.tsto the Objects bundle allowlist.
- Follow-up: if
batchcontextlivecounter.ts/batchcontextlivemap.tswere removed, consider pruning them here as well to avoid “referenced in allowedFiles but does not contribute” errors at runtime.ably.d.ts (3)
2685-2731: AnyPathObject shape looks coherent; minor generic hygiene.Interface surface matches the runtime impl (get/value/instance/compact) and adds collection helpers. Consider avoiding generic name reuse downstream (see AnyOperations comment) to keep inference clearer across chained calls.
2981-2993: TypeDoc noise from mapped/conditional types — hide or alias.The
BatchContextOperations<T>mapped/conditional type often triggers TypeDoc “__type/this/args” warnings. Two options:
- Mark it
@internaland expose a named interface alias for docs, or- Wrap with an exported alias and document that alias instead.
This should address the current API Reference warnings.
3124-3219: AnyOperations: avoid generic shadowing and tighten key/value generics.Methods reuse the generic name
T, which collides with the interface’s ambient types and can confuse inference inside consumers.Suggested tweaks:
- Rename method-level generics (
set/remove) toM(or similar).- Keep
batchgeneric as-is (LiveObject-only) — OK.Example:
- set<T extends Record<string, Value> = Record<string, Value>>(key: keyof T & string, value: T[keyof T]): Promise<void>; + set<M extends Record<string, Value> = Record<string, Value>>(key: keyof M & string, value: M[keyof M]): Promise<void>; - remove<T extends Record<string, Value> = Record<string, Value>>(key: keyof T & string): Promise<void>; + remove<M extends Record<string, Value> = Record<string, Value>>(key: keyof M & string): Promise<void>;src/plugins/objects/instance.ts (1)
194-206: Batch lifecycle looks correct; consider renaming the method generic.Validation +
RootBatchContextcreation +flush()+finally close()is sound. To avoid generic shadowing with the class’sT, rename the method’s generic toU:- async batch<T extends LiveObjectType = LiveObjectType>(fn: BatchFunction<T>): Promise<void> { + async batch<U extends LiveObjectType = LiveObjectType>(fn: BatchFunction<U>): Promise<void> { - fn(ctx as unknown as BatchContext<T>); + fn(ctx as unknown as BatchContext<U>);src/plugins/objects/pathobject.ts (1)
374-391: PathObject.batch mirrors Instance.batch — good; rename method generic to avoid shadowing.Control flow and error messaging (path-aware) are solid. Minor: rename the method generic to
Ufor clarity:- async batch<T extends LiveObjectType = LiveObjectType>(fn: BatchFunction<T>): Promise<void> { + async batch<U extends LiveObjectType = LiveObjectType>(fn: BatchFunction<U>): Promise<void> { - fn(ctx as unknown as BatchContext<T>); + fn(ctx as unknown as BatchContext<U>);src/plugins/objects/rootbatchcontext.ts (2)
66-68: Minor: prefer const for wrappedInstanceUse const; the binding isn’t reassigned.
- let wrappedInstance = new DefaultBatchContext(this._realtimeObject, instance, this); + const wrappedInstance = new DefaultBatchContext(this._realtimeObject, instance, this);
6-9: Type duplication: centralize InstanceEventInstanceEvent is also declared in batchcontext.ts. Consider defining it once (e.g., a shared types module) and importing to avoid drift.
src/plugins/objects/batchcontext.ts (3)
75-83: Capture objectId before queuingAvoid reading this._instance.id() at flush-time; capture while context is open.
set(key: string, value: Value): void { this._throwIfClosed(); if (!(this._instance as DefaultInstance<Value>).isLiveMap()) { throw new this._client.ErrorInfo('Cannot set a key on a non-LiveMap instance', 92007, 400); } - this._rootContext.queueMessages(async () => - LiveMap.createMapSetMessage(this._realtimeObject, this._instance.id()!, key, value as Primitive), - ); + const objectId = this._instance.id()!; + this._rootContext.queueMessages(() => + LiveMap.createMapSetMessage(this._realtimeObject, objectId, key, value as Primitive), + ); }
85-93: Same here: capture objectId for remove()Pre-read objectId to keep ctors simple and robust post-close.
remove(key: string): void { this._throwIfClosed(); if (!(this._instance as DefaultInstance<Value>).isLiveMap()) { throw new this._client.ErrorInfo('Cannot remove a key from a non-LiveMap instance', 92007, 400); } - this._rootContext.queueMessages(async () => [ - LiveMap.createMapRemoveMessage(this._realtimeObject, this._instance.id()!, key), - ]); + const objectId = this._instance.id()!; + this._rootContext.queueMessages(async () => [ + LiveMap.createMapRemoveMessage(this._realtimeObject, objectId, key), + ]); }
95-103: And for increment(): capture objectId and deltaKeeps all dependent values stable when ctors run.
increment(amount?: number): void { this._throwIfClosed(); if (!(this._instance as DefaultInstance<Value>).isLiveCounter()) { throw new this._client.ErrorInfo('Cannot increment a non-LiveCounter instance', 92007, 400); } - this._rootContext.queueMessages(async () => [ - LiveCounter.createCounterIncMessage(this._realtimeObject, this._instance.id()!, amount ?? 1), - ]); + const objectId = this._instance.id()!; + const delta = amount ?? 1; + this._rootContext.queueMessages(async () => [ + LiveCounter.createCounterIncMessage(this._realtimeObject, objectId, delta), + ]); }
📜 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 (11)
ably.d.ts(9 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/batchcontextlivecounter.ts(0 hunks)src/plugins/objects/batchcontextlivemap.ts(0 hunks)src/plugins/objects/instance.ts(4 hunks)src/plugins/objects/livemap.ts(3 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/realtimeobject.ts(0 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)test/realtime/objects.test.js(17 hunks)
💤 Files with no reviewable changes (3)
- src/plugins/objects/realtimeobject.ts
- src/plugins/objects/batchcontextlivemap.ts
- src/plugins/objects/batchcontextlivecounter.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/objects/livemap.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
LiveMap(2377-2380)BatchFunction(1672-1672)LiveObject(2393-2393)BatchContext(3004-3010)LiveCounter(2385-2388)src/plugins/objects/livemap.ts (1)
LiveMap(49-994)src/plugins/objects/rootbatchcontext.ts (1)
RootBatchContext(11-78)
test/realtime/objects.test.js (2)
src/plugins/objects/livecounter.ts (1)
LiveCounter(15-301)src/plugins/objects/livemap.ts (1)
LiveMap(49-994)
src/plugins/objects/batchcontext.ts (4)
src/plugins/objects/instance.ts (3)
InstanceEvent(22-25)value(76-104)DefaultInstance(27-217)src/plugins/objects/rootbatchcontext.ts (2)
InstanceEvent(6-9)RootBatchContext(11-78)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/livemap.ts (1)
LiveMap(49-994)
src/plugins/objects/pathobject.ts (3)
ably.d.ts (3)
LiveMap(2377-2380)BatchFunction(1672-1672)BatchContext(3004-3010)src/plugins/objects/livemap.ts (1)
LiveMap(49-994)src/plugins/objects/rootbatchcontext.ts (1)
RootBatchContext(11-78)
src/plugins/objects/rootbatchcontext.ts (3)
src/plugins/objects/batchcontext.ts (1)
DefaultBatchContext(15-118)ably.d.ts (3)
ObjectMessage(3614-3661)Instance(3512-3518)Value(2400-2400)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(38-502)
ably.d.ts (3)
src/plugins/objects/batchcontext.ts (1)
value(35-38)src/plugins/objects/instance.ts (1)
value(76-104)src/plugins/objects/pathobject.ts (1)
value(145-184)
🪛 GitHub Actions: API Reference
ably.d.ts
[warning] 1-1: TypeDoc warning: member lacks documentation in ably.d.ts (e.g., set, remove, __type, __type.this, __type.args, etc.).
⏰ 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 (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (7)
ably.d.ts (3)
1666-1672: BatchFunction is sync-only — LGTM.Typing
BatchFunction<T> = (ctx) => voidcorrectly preventsasyncfunctions at compile time and matches the runtime batching contract.
3039-3083: Batch semantics docs are clear — LGTM.The “added to current batch if called within batch()” behavior is well documented for
LiveMapOperations.set/remove.
3088-3118: Counter ops batch semantics — LGTM.Docs mirror the map ops; errors for wrong-type calls are explicit.
src/plugins/objects/instance.ts (2)
118-125: keys(): safe early return for non-Map — LGTM.Returning an empty iterator when not a LiveMap avoids accidental throws.
208-216: Internal helpers are useful for narrowing — LGTM.
isLiveMap()/isLiveCounter()aid internal checks without leaking API surface.src/plugins/objects/pathobject.ts (1)
237-254: keys(): tolerant resolution — LGTM.Gracefully handling non-Map resolutions and 92005 path errors matches the rest of the API’s undefined/empty-iterator convention.
src/plugins/objects/rootbatchcontext.ts (1)
31-45: Flush safety verified for queued message constructors They only call this._instance.id() and static message-creator methods, avoiding any guarded BatchContext APIs.
| * BatchContextBase defines the set of common methods on a BatchContext | ||
| * that are present regardless of the underlying type. | ||
| */ | ||
| interface BatchContextBase { | ||
| /** | ||
| * Get the object ID of the underlying instance. | ||
| * | ||
| * If the underlying instance at runtime is not a {@link LiveObject}, returns `undefined`. | ||
| * | ||
| * @experimental | ||
| */ | ||
| id(): string | undefined; | ||
| } | ||
|
|
There was a problem hiding this comment.
PrimitiveBatchContext should also include BatchContextBase for a uniform API.
Docs say BatchContextBase methods are “present regardless of the underlying type,” but PrimitiveBatchContext doesn’t extend it, so id() is missing on primitive contexts (where it should return undefined). Align types for consistency.
Suggested diff:
-export interface PrimitiveBatchContext<T extends Primitive = Primitive> {
+export interface PrimitiveBatchContext<T extends Primitive = Primitive> extends BatchContextBase {
/**
* Get the underlying primitive value.
* ...
*/
value(): T | undefined;
/**
* Get a JavaScript object representation of the primitive value.
* ...
*/
compact(): CompactedValue<T> | undefined;
}Also applies to: 2866-2884
🤖 Prompt for AI Agents
In ably.d.ts around lines 2750-2763 (and similarly 2866-2884),
PrimitiveBatchContext does not extend BatchContextBase so it lacks the id()
method; update the PrimitiveBatchContext interface to extend BatchContextBase
(or include id(): string | undefined) so primitive contexts expose id()
returning undefined when not a LiveObject, preserving the uniform API across
BatchContext types.
There was a problem hiding this comment.
primitive values do not have ids, so it should not
There was a problem hiding this comment.
@VeskeR, you're absolutely right. The current design is intentional and consistent throughout the codebase.
PrimitiveBatchContext doesn't extend BatchContextBase for the same reason that PrimitiveInstance doesn't extend InstanceBase — primitive values are snapshots that don't have object IDs, so exposing an id() method (even one that returns undefined) would be semantically misleading.
Looking at the type definitions, this pattern is applied consistently:
- Collection types (
LiveMapBatchContext,LiveCounterBatchContext) exposeid()because they wrap tracked objects - Primitive wrappers (
PrimitiveBatchContext,PrimitiveInstance) omitid()because they represent values, not identifiable objects - Generic fallback types (
AnyBatchContext,AnyInstance) includeid()because they must handle all cases
This design makes the API clearer by only exposing id() where it's conceptually meaningful. The CodeRabbit suggestion appears to be a false positive.
✏️ Learnings added
Learnt from: VeskeR
Repo: ably/ably-js PR: 2107
File: ably.d.ts:2750-2763
Timestamp: 2025-11-07T17:53:25.083Z
Learning: In ably.d.ts, PrimitiveBatchContext and PrimitiveInstance intentionally do not extend BatchContextBase or InstanceBase (respectively) and do not expose an id() method. This is by design: primitive values are snapshots/values without object identity, so omitting id() makes the API semantically clearer. Only collection types (LiveMapBatchContext, LiveCounterBatchContext, LiveMapInstance, LiveCounterInstance) and generic fallback types (AnyBatchContext, AnyInstance) should expose id().
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
This also removes the now-obsolete RealtimeObject.batch() API.
Resolves PUB-2065
Summary by CodeRabbit
New Features
Refactor