[PUB-2064] Implement .compact() method for PathObject and Instance types for LiveObjects
#2098
Conversation
WalkthroughAdds a typed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant PO as PathObject
participant Resolver as Path Resolver
participant LM as LiveMap
note over Caller,PO: PathObject.compact()
Caller->>PO: compact()
PO->>Resolver: resolve(path)
alt resolved LiveMap
Resolver-->>PO: LiveMap instance
PO->>LM: compact()
LM-->>PO: Plain object (recursively compacted)
PO-->>Caller: CompactedValue
else resolved LiveCounter
Resolver-->>PO: LiveCounter
PO-->>Caller: number (from value())
else resolved primitive/buffer/json
Resolver-->>PO: primitive/buffer/json
PO-->>Caller: primitive/encoded value
else not found / ErrorInfo
Resolver-->>PO: ErrorInfo (e.g., 92005)
PO-->>Caller: undefined
end
sequenceDiagram
autonumber
participant Caller as Caller
participant Inst as DefaultInstance
participant LM as LiveMap
note over Caller,Inst: Instance.compact()
Caller->>Inst: compact()
alt underlying LiveMap
Inst->>LM: compact()
LM-->>Inst: Plain object
Inst-->>Caller: CompactedValue
else other underlying value
Inst-->>Caller: value() as CompactedValue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 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/2098/features
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/objects/pathobject.ts (1)
134-138: Resolveinstance()TypeScript error
DefaultInstanceis inferred asDefaultInstance<Value & LiveObject<…>>, which doesn’t satisfyAnyInstance<T>, triggering the TS2322 failures seen in CI. Cast or parameterise the newDefaultInstanceso the returned type matchesAnyInstance<T>(e.g.return new DefaultInstance(this._realtimeObject, value) as unknown as AnyInstance<T>;).
📜 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 (5)
ably.d.ts(11 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livemap.ts(2 hunks)src/plugins/objects/pathobject.ts(2 hunks)test/realtime/objects.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/objects/instance.ts (2)
ably.d.ts (3)
Value(2458-2458)CompactedValue(2464-2480)LiveMap(2435-2438)src/plugins/objects/livemap.ts (1)
LiveMap(52-1130)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (3)
Value(2458-2458)CompactedValue(2464-2480)LiveMap(2435-2438)src/plugins/objects/livemap.ts (1)
LiveMap(52-1130)
src/plugins/objects/livemap.ts (4)
src/plugins/objects/instance.ts (1)
value(65-87)src/plugins/objects/pathobject.ts (1)
value(96-129)src/plugins/objects/livecounter.ts (2)
value(135-138)LiveCounter(23-374)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
ably.d.ts (3)
src/plugins/objects/livemap.ts (1)
LiveMap(52-1130)objects.d.ts (2)
LiveMap(16-29)LiveCounter(34-45)src/plugins/objects/livecounter.ts (1)
LiveCounter(23-374)
test/realtime/objects.test.js (5)
src/plugins/objects/instance.ts (1)
value(65-87)src/plugins/objects/pathobject.ts (2)
value(96-129)instance(131-150)src/plugins/objects/livecounter.ts (2)
value(135-138)LiveCounter(23-374)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livemap.ts (1)
LiveMap(52-1130)
🪛 GitHub Actions: API Reference
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. Type '(listener: EventCallback<InstanceSubscriptionEvent<Value & LiveObject<any, any>>>) => SubscribeResponse' is not assignable to type '(listener: EventCallback<InstanceSubscriptionEvent>) => SubscribeResponse'.
🪛 GitHub Actions: Bundle size report
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. Type '(listener: EventCallback<InstanceSubscriptionEvent<Value & LiveObject<any, any>>>) => SubscribeResponse' is not assignable to type '(listener: EventCallback<InstanceSubscriptionEvent>) => SubscribeResponse'. Types of parameters 'listener' and 'listener' are incompatible. Type 'InstanceSubscriptionEvent<Value & LiveObject<any, any>>' is not assignable to type 'InstanceSubscriptionEvent'. 'Value & LiveObject<any, any>' is not assignable to type 'T'. 'Value & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'. Type 'string & LiveObject<any, any>' is not assignable to type 'T'. 'string & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'.
🪛 GitHub Actions: Lint
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Incompatible 'subscribe' signature between Value & LiveObject and T.
🪛 GitHub Actions: Spec coverage report
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. Type '(listener: EventCallback<InstanceSubscriptionEvent<Value & LiveObject<any, any>>>) => SubscribeResponse' is not assignable to type '(listener: EventCallback<InstanceSubscriptionEvent>) => SubscribeResponse'. Types of parameters 'listener' and 'listener' are incompatible. Types of parameters 'event' and 'event' are incompatible. Type 'InstanceSubscriptionEvent<Value & LiveObject<any, any>>' is not assignable to type 'InstanceSubscriptionEvent'. Type 'Value & LiveObject<any, any>' is not assignable to type 'T'. 'Value & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'. Type 'string & LiveObject<any, any>' is not assignable to type 'T'. 'string & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'.
🪛 GitHub Actions: Test (react hooks)
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. Type '(listener: EventCallback<InstanceSubscriptionEvent<Value & LiveObject<any, any>>>) => SubscribeResponse' is not assignable to type '(listener: EventCallback<InstanceSubscriptionEvent>) => SubscribeResponse'. Types of parameters 'listener' and 'listener' are incompatible. Types of parameters 'event' and 'event' are incompatible. Type 'InstanceSubscriptionEvent<Value & LiveObject<any, any>>' is not assignable to type 'InstanceSubscriptionEvent'. Type 'Value & LiveObject<any, any>' is not assignable to type 'T'. 'Value & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'. Type 'string & LiveObject<any, any>' is not assignable to type 'T'. 'string & LiveObject<any, any>' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint 'Value'.
🪛 GitHub Actions: Test browser
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. Type '(listener: EventCallback<InstanceSubscriptionEvent<Value & LiveObject<any, any>>>) => SubscribeResponse' is not assignable to type '(listener: EventCallback<InstanceSubscriptionEvent>) => SubscribeResponse'.
🪛 GitHub Actions: Test NodeJS
src/plugins/objects/pathobject.ts
[error] 137-137: TS2322: Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. Types of property 'subscribe' are incompatible. See log for details.
🪛 GitHub Actions: Test NPM package
src/plugins/objects/pathobject.ts
[error] 137-137: TypeScript error TS2322 in pathobject.ts(137,9): Type 'DefaultInstance<Value & LiveObject<any, any>>' is not assignable to type 'AnyInstance'. subscribe signatures are incompatible.
[error] Build failed due to TypeScript compilation error in pathobject.ts. See above TS2322 for details.
🔇 Additional comments (1)
test/realtime/objects.test.js (1)
4887-5880: Great coverage on compact() behaviorAppreciate how these scenarios mirror value() expectations across primitives, counters, nested maps, and verify PathObject vs DefaultInstance parity. This should give us strong confidence in the new compact() contract.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
953bb0f to
113304e
Compare
VeskeR
force-pushed
the
PUB-2064/compact-representation
branch
from
ff351e2 to
a9a0ec9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2098/features
github-actions
bot
temporarily deployed
to
staging/pull/2098/bundle-report
VeskeR
force-pushed
the
PUB-2064/compact-representation
branch
from
a9a0ec9 to
eb94121
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2098/features
github-actions
bot
temporarily deployed
to
staging/pull/2098/bundle-report
VeskeR
changed the title
.compact() method for PathObject and Instance types for LiveObjects
src/plugins/objects/pathobject.ts
Outdated
|
|
||
| return this.value() as CompactedValue<U>; | ||
| } catch (error) { | ||
| if (this._client.Utils.isErrorInfoOrPartialErrorInfo(error)) { |
There was a problem hiding this comment.
This should assert on error code 92005 after #2092 is merged
There was a problem hiding this comment.
| } | ||
|
|
||
| // Other values return as is | ||
| result[key] = value; |
There was a problem hiding this comment.
I think buffer values should be base64 encoded strings; I think the object returned from compact() should be a trivially JSON-encodable object
There was a problem hiding this comment.
made binary values return base64 encoded strings in 94be648
This makes LiveObjects Instance methods to be in line with PathObject methods handling of wrong underlying type, as discussed in [1]. Type/behavior overview: - id(): may return `undefined` (when Instance is from .get() call that returned a primitive instance) - compact(): is defined for all Instance types so can be non-nullable. See method implementation in [2] - get(): may return `undefined` when not a map - entries()/keys()/values()/size(): may return `undefined` when not a map - value(): may return `undefined` when not a counter or a primitive [1] #2091 (comment) [2] #2098
This makes LiveObjects Instance methods to be in line with PathObject methods handling of wrong underlying type, as discussed in [1]. Type/behavior overview: - id(): may return `undefined` (when Instance is from .get() call that returned a primitive instance) - compact(): is defined for all Instance types so can be non-nullable. See method implementation in [2] - get(): may return `undefined` when not a map - entries()/keys()/values()/size(): may return `undefined` when not a map - value(): may return `undefined` when not a counter or a primitive [1] #2091 (comment) [2] #2098
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
5 times, most recently
from
a691e56 to
6024f4b
Compare
mschristensen
left a comment
mschristensen
left a comment
There was a problem hiding this comment.
Minor comment but otherwise looks good :)
| 'DefaultInstance.value()', | ||
| `unexpected value type for instance, resolving to undefined; value=${this._value}`, | ||
| ); | ||
| // unknown type - return undefined |
There was a problem hiding this comment.
Can we log the type of this_value here? We should never encounter this case so if we do we should have a log to help us understand the bug
There was a problem hiding this comment.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
2 times, most recently
from
07ec19d to
5406c87
Compare
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
5406c87 to
3a940c6
Compare
VeskeR
force-pushed
the
PUB-2064/compact-representation
branch
from
eb94121 to
ab56749
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2098/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2098/features
github-actions
bot
temporarily deployed
to
staging/pull/2098/typedoc
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 (1)
src/plugins/objects/realtimeobject.ts (1)
98-108: Add explicit cast to match PathObject<LiveMap> return type.The type mismatch is real and must be fixed.
DefaultPathObjectimplements the broadAnyPathObjectinterface, butgetPathObjectpromisesPromise<API.PathObject<API.LiveMap<T>>>, which resolves to the specificLiveMapPathObject<T>interface. These are structurally incompatible without casting (LiveMapPathObject has key-awareget<K extends keyof T>(), while AnyPathObject has generic operations).The proposed cast pattern matches the established convention in
pathobject.ts(lines 80, 126, 207–209):- const pathObject = new DefaultPathObject(this, this._objectsPool.getRoot(), []); - return pathObject; + const pathObject = + new DefaultPathObject(this, this._objectsPool.getRoot(), []) as unknown as API.PathObject<API.LiveMap<T>>; + return pathObject;
♻️ Duplicate comments (2)
src/plugins/objects/livemap.ts (1)
563-566: LiveCounter branch fallthrough fixed.The added
continuepreserves the numeric compacted value.src/plugins/objects/pathobject.ts (1)
53-70: Inconsistent error handling - should check for error code 92005.The error handling catches all
ErrorInfoinstances, but the comment indicates only path resolution failures should returnundefined. Other methods in this file (e.g.,value()at line 165,instance()at line 186) specifically check for error code92005before returningundefined.Apply this diff to align with the error handling pattern used elsewhere:
} catch (error) { - if (this._client.Utils.isErrorInfoOrPartialErrorInfo(error)) { - // ignore ErrorInfos indicating path resolution failure and return undefined + if (this._client.Utils.isErrorInfoOrPartialErrorInfo(error) && error.code === 92005) { + // ignore path resolution errors and return undefined return undefined; } - // otherwise rethrow unexpected errors + // rethrow everything else throw error; }
🧹 Nitpick comments (3)
ably.d.ts (2)
2418-2439: Simplify CompactedValue; drop unreachable undefined branches and avoidany.
T extends Valueexcludesundefined, so the| undefinedarms never match.- Prefer
unknownoveranyto avoid masking type errors.Suggested rewrite:
-export type CompactedValue<T extends Value> = - // LiveMap types - [T] extends [LiveMap<infer U>] - ? { [K in keyof U]: CompactedValue<U[K]> } - : [T] extends [LiveMap<infer U> | undefined] - ? { [K in keyof U]: CompactedValue<U[K]> } | undefined - : // LiveCounter types - [T] extends [LiveCounter] - ? number - : [T] extends [LiveCounter | undefined] - ? number | undefined - : // Primitive types - [T] extends [Primitive] - ? T - : [T] extends [Primitive | undefined] - ? T - : any; +export type CompactedValue<T extends Value> = + T extends LiveMap<infer U> ? { [K in keyof U]: CompactedValue<U[K]> } : + T extends LiveCounter ? number : + T extends Primitive ? T : + unknown;Please confirm there are no external call sites relying on
CompactedValue<... | undefined>; methods already model| undefinedat the return type level.
2693-2700: Tighten AnyPathObject.value generic (remove redundantnumber).
numberis already part ofPrimitive; use a single constraint for clarity:- value<T extends number | Primitive = number | Primitive>(): T | undefined; + value<T extends Primitive = Primitive>(): T | undefined;Scan for downstream generic arguments that explicitly pass
number; they remain valid sincenumberextendsPrimitive.src/plugins/objects/livemap.ts (1)
547-574: Consider making compact() output JSON-encodable (encode binary values).Currently, Buffer/ArrayBuffer values are copied through. If “compact” is intended for JSON transport or persistence, encode binary (e.g., base64 string) and document the shape. Otherwise, document that binary remains binary to set expectations.
Confirm product intent for
compact()outputs. If JSON-encodable is desired, I can propose a concrete implementation using the existing encoding utilities.
📜 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 (6)
ably.d.ts(12 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livemap.ts(2 hunks)src/plugins/objects/pathobject.ts(5 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)test/realtime/objects.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/instance.ts (2)
ably.d.ts (3)
Value(2416-2416)CompactedValue(2422-2438)LiveMap(2393-2396)src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)
src/plugins/objects/livemap.ts (4)
src/plugins/objects/instance.ts (1)
value(66-94)src/plugins/objects/pathobject.ts (1)
value(133-172)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
src/plugins/objects/realtimeobject.ts (1)
src/plugins/objects/pathobject.ts (1)
DefaultPathObject(25-376)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (5)
AnyPathObject(2678-2716)LiveMap(2393-2396)Value(2416-2416)CompactedValue(2422-2438)PathObject(2725-2731)src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)
test/realtime/objects.test.js (4)
src/plugins/objects/instance.ts (1)
value(66-94)src/plugins/objects/pathobject.ts (2)
value(133-172)instance(174-193)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)
ably.d.ts (3)
src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)objects.d.ts (2)
LiveMap(16-28)LiveCounter(33-43)src/plugins/objects/livecounter.ts (1)
LiveCounter(15-301)
⏰ 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 (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (6)
src/plugins/objects/instance.ts (1)
41-47: LGTM! Clean implementation of compact representation.The method correctly delegates to
LiveMap.compact()for map types and falls back tovalue()for primitives andLiveCounter, which aligns with theCompactedValue<T>type definition. The logic is straightforward and type-safe assuming callers provide the correct generic parameterU.src/plugins/objects/pathobject.ts (5)
25-25: LGTM! Non-generic class design simplifies path tracking.The change from a generic to a non-generic
DefaultPathObjectclass simplifies the implementation while maintaining type safety through casts at method return sites (lines 80, 126). This aligns with the path-tracking approach where the actual type is resolved dynamically at runtime.
27-38: LGTM! Path tracking correctly accumulates from parent.The constructor properly builds the full path by concatenating the parent's path with new segments. The use of spread operators ensures immutability, and optional chaining safely handles the case when no parent is provided.
40-48: LGTM! Proper path escaping for dot-containing segments.The method correctly escapes dots within path segments to avoid ambiguity in the joined path string. This ensures segments like "a.b" are represented as "a.b" in the final path.
76-81: LGTM! Correctly passes parent for path tracking.The method now passes
thisas the parent parameter toDefaultPathObject, enabling proper path accumulation in the constructor. The cast toPathObject<U>maintains the expected return type.
86-127: LGTM! Correctly passes parent for path tracking.The method now passes
thisas the parent parameter toDefaultPathObject, enabling proper path accumulation. This is consistent with theget()method's approach.
VeskeR
force-pushed
the
PUB-2064/compact-representation
branch
from
ab56749 to
f3eee80
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2098/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2098/features
github-actions
bot
temporarily deployed
to
staging/pull/2098/typedoc
…eObjects Resolves PUB-2064
VeskeR
force-pushed
the
PUB-2064/compact-representation
branch
from
f3eee80 to
b1d0234
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2098/features
github-actions
bot
temporarily deployed
to
staging/pull/2098/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2098/typedoc
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/realtime/objects.test.js (3)
4996-5032: Use BufferUtils for Buffer equality in compact() primitive testsDeep equality on Buffers can be environment-sensitive; compare bytes explicitly for robustness.
Apply this diff inside the primitive loop:
- expect(compactValue).to.deep.equal( - expectedValue, - `Check PathObject.compact() returns correct value for primitive "${keyData.key}"`, - ); + if (keyData.data.bytes != null) { + helper.recordPrivateApi('call.BufferUtils.areBuffersEqual'); + expect( + BufferUtils.areBuffersEqual(compactValue, expectedValue), + `Check PathObject.compact() returns correct bytes for primitive "${keyData.key}"`, + ).to.be.true; + } else { + expect( + compactValue, + ).to.deep.equal( + expectedValue, + `Check PathObject.compact() returns correct value for primitive "${keyData.key}"`, + ); + }
5762-5798: Use BufferUtils for Buffer equality in DefaultInstance compact() primitive testsMirror the Buffer comparison improvement here as well.
Apply this diff inside the primitive loop:
- expect(compactValue).to.deep.equal( - expectedValue, - `Check DefaultInstance.compact() returns correct value for primitive "${keyData.key}"`, - ); + if (keyData.data.bytes != null) { + helper.recordPrivateApi('call.BufferUtils.areBuffersEqual'); + expect( + BufferUtils.areBuffersEqual(compactValue, expectedValue), + `Check DefaultInstance.compact() returns correct bytes for primitive "${keyData.key}"`, + ).to.be.true; + } else { + expect( + compactValue, + ).to.deep.equal( + expectedValue, + `Check DefaultInstance.compact() returns correct value for primitive "${keyData.key}"`, + ); + }
5049-5080: Optional: add coverage for empty maps and tombstoned entriesCompact uses entries() which excludes tombstoned keys; add explicit checks and empty-map case to lock behavior.
You can append scenarios like:
{ description: 'PathObject.compact() excludes tombstoned entries and compacts empty maps to {}', action: async ({ entryPathObject, entryInstance }) => { let p = waitForMapKeyUpdate(entryInstance, 'm'); await entryPathObject.set('m', LiveMap.create({ a: 'x' })); await p; expect(entryPathObject.get('m').compact()).to.deep.equal({ a: 'x' }); p = waitForMapKeyUpdate(entryInstance.get('m'), 'a'); await entryPathObject.get('m').remove('a'); await p; expect(entryPathObject.get('m').compact()).to.deep.equal({}); }, }And similarly for DefaultInstance:
{ description: 'DefaultInstance.compact() excludes tombstoned entries and compacts empty maps to {}', action: async ({ entryPathObject, entryInstance }) => { let p = waitForMapKeyUpdate(entryInstance, 'm2'); await entryPathObject.set('m2', LiveMap.create({ k: 'v' })); await p; expect(entryInstance.get('m2').compact()).to.deep.equal({ k: 'v' }); p = waitForMapKeyUpdate(entryInstance.get('m2'), 'k'); await entryPathObject.get('m2').remove('k'); await p; expect(entryInstance.get('m2').compact()).to.deep.equal({}); }, }ably.d.ts (1)
2418-2438: Consider implications of 'any' fallback in CompactedValue.The CompactedValue type definition correctly handles LiveMap, LiveCounter, and Primitive types with proper recursion and undefined handling. However, the fallback case at line 2438 returns
anyfor unmatched types:: any;While this fallback is likely necessary for the
AnyPathObjectandAnyInstanceuse cases where the concrete type is not known at compile time, it means that passing an unexpected type parameter will silently returnanyrather than causing a type error.This is probably acceptable given the design requirements, but be aware that it reduces type safety for edge cases. If stricter typing is desired in the future, consider whether these cases should return
neveror require explicit handling.
📜 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 (6)
ably.d.ts(12 hunks)src/plugins/objects/instance.ts(3 hunks)src/plugins/objects/livemap.ts(2 hunks)src/plugins/objects/pathobject.ts(5 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)test/realtime/objects.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/objects/instance.ts
- src/plugins/objects/realtimeobject.ts
🧰 Additional context used
🧬 Code graph analysis (4)
test/realtime/objects.test.js (5)
src/plugins/objects/instance.ts (1)
value(66-94)src/plugins/objects/pathobject.ts (1)
value(134-173)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (5)
AnyPathObject(2678-2716)LiveMap(2393-2396)Value(2416-2416)CompactedValue(2422-2438)PathObject(2725-2731)src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)
ably.d.ts (2)
src/plugins/objects/livemap.ts (1)
LiveMap(49-1031)src/plugins/objects/livecounter.ts (1)
LiveCounter(15-301)
src/plugins/objects/livemap.ts (4)
src/plugins/objects/instance.ts (1)
value(66-94)src/plugins/objects/pathobject.ts (1)
value(134-173)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
⏰ 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 (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (10)
src/plugins/objects/livemap.ts (1)
4-4: LGTM!The import is necessary to support the
instanceof LiveCountercheck andvalue()call in the newcompact()method.test/realtime/objects.test.js (9)
4296-4297: Compact returns undefined for non-existent path — LGTMSilent undefined matches PathObject API for unresolved paths.
4344-4345: Compact returns undefined for non-collection intermediate — LGTMConsistent with non-collection traversal semantics.
5035-5046: LiveCounter compacts to number — LGTMMatches counter.value() behavior.
5049-5080: LiveMap compacts to plain object — LGTMRecurses for LiveMap and inlines counter.value() as expected.
5083-5116: Complex nested compact for PathObject — LGTMStructure and values align with recursive compact semantics.
5801-5812: DefaultInstance counter compacts to number — LGTMBehavior matches expectations.
5815-5846: DefaultInstance LiveMap compacts to plain object — LGTMConsistent with LiveMap.compact recursion.
5849-5883: Complex nested compact for DefaultInstance — LGTMDeep structure verified correctly.
5886-5912: Equivalence: PathObject.compact vs DefaultInstance.compact — LGTMGood cross-surface sanity check.
…s for binary values
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/plugins/objects/instance.ts (1)
95-95: Logging now includes value and typeof — LGTM.Captures unexpected cases with useful context.
src/plugins/objects/pathobject.ts (1)
71-74: Clarify 92005 in the comment.Add a brief note so readers know 92005 is the “unresolvable path” code:
- // ignore path resolution errors and return undefined + // 92005 indicates the path cannot be resolved (see _resolvePath); ignore and return undefinedsrc/plugins/objects/livemap.ts (1)
569-577: Base64-encode ArrayBuffer values too (keep compact() trivially JSON-encodable).Buffer is encoded, but ArrayBuffer is returned as-is. ably.d.ts’ CompactedValue maps ArrayBuffer to string, so this is a runtime/type mismatch and breaks JSON.stringify in browsers.
Apply this diff:
if (this._client.Platform.BufferUtils.isBuffer(value)) { result[key] = this._client.Platform.BufferUtils.base64Encode(value); continue; } + if (value instanceof ArrayBuffer) { + result[key] = this._client.Platform.BufferUtils.base64Encode(value); + continue; + } + // Other values return as is result[key] = value;Please confirm tests cover ArrayBuffer in LiveMap.compact().
🧹 Nitpick comments (2)
src/plugins/objects/livemap.ts (1)
554-555: Optional: tighten return type to match public API.Return type can mirror the public type for clarity and safety:
- compact(): { [K in keyof T]: any } { + compact(): API.CompactedValue<API.LiveMap<T>> {No behavior change; improves parity with ably.d.ts.
ably.d.ts (1)
2418-2448: Runtime must align with CompactedValue binary mapping.CompactedValue maps Buffer and ArrayBuffer to string. Please ensure all compact() implementations (LiveMap, PathObject, Instance) base64-encode both Buffer and ArrayBuffer. The proposed code changes in src cover this.
Optional: document that LiveMap.compact() returns only present entries at runtime, even though the mapped type lists all keys.
📜 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 (5)
ably.d.ts(12 hunks)src/plugins/objects/instance.ts(3 hunks)src/plugins/objects/livemap.ts(2 hunks)src/plugins/objects/pathobject.ts(5 hunks)test/realtime/objects.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/realtime/objects.test.js
🧰 Additional context used
🧬 Code graph analysis (4)
src/plugins/objects/pathobject.ts (3)
ably.d.ts (5)
AnyPathObject(2694-2735)LiveMap(2393-2396)Value(2416-2416)CompactedValue(2422-2447)PathObject(2744-2750)src/plugins/objects/livemap.ts (1)
LiveMap(49-1038)src/plugins/objects/instance.ts (1)
value(72-100)
src/plugins/objects/livemap.ts (4)
src/plugins/objects/instance.ts (1)
value(72-100)src/plugins/objects/pathobject.ts (1)
value(141-180)src/plugins/objects/livecounter.ts (2)
value(64-67)LiveCounter(15-301)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
src/plugins/objects/instance.ts (2)
ably.d.ts (3)
Value(2416-2416)CompactedValue(2422-2447)LiveMap(2393-2396)src/plugins/objects/livemap.ts (1)
LiveMap(49-1038)
ably.d.ts (1)
src/plugins/objects/livemap.ts (1)
LiveMap(49-1038)
⏰ 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 (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (2)
src/plugins/objects/instance.ts (1)
41-53: No changes needed—ArrayBuffer is already handled correctly.The existing
isBuffer()check on both platforms (web and Node.js) already includesinstanceof ArrayBuffer, so ArrayBuffer values are properly encoded to base64 strings viabase64Encode(). The suggested diff is redundant.src/plugins/objects/pathobject.ts (1)
55-77: Review comment is incorrect regarding ArrayBuffer handling—code already handles it.The current implementation already encodes ArrayBuffer values to base64. The
isBuffer()check at line 65 returnstruefor both Buffer and ArrayBuffer (it checksbuffer instanceof ArrayBuffer || ArrayBuffer.isView(buffer)in web, and includes ArrayBuffer in Node.js). Whenbase64Encode()is called on an ArrayBuffer,toBuffer()converts it to a Uint8Array before encoding.The proposed diff adding an explicit
instanceof ArrayBuffercheck is redundant—this case is already covered by the existingisBuffer()condition.The double resolution concern (calling
_resolvePath()twice: once incompact()at line 57, then again invalue()at line 143) is technically valid but represents an optional efficiency improvement rather than a bug.Likely an incorrect or invalid review comment.
VeskeR
merged commit 9b37248
into
integration/objects-breaking-api
Resolves PUB-2064
Summary by CodeRabbit
New Features
Tests