[PUB-2057] Implement base PathObject class with access and mutation methods #2091
Conversation
WalkthroughAdds a PathObject API with DefaultPathObject implementation and RealtimeObject.getPathObject(); expands the public type surface to parallel deprecated variants (LiveMapDeprecated, LiveObjectDeprecated, LiveCounterDeprecated); includes pathobject.ts in bundle checks and extensive PathObject tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Application
participant RO as RealtimeObject
participant Sync as Initial Sync
participant DPO as DefaultPathObject
participant Root as Root LiveMapDeprecated
App->>RO: getPathObject<T>()
RO->>Sync: ensure initial sync
Sync-->>RO: synced
RO->>DPO: new DefaultPathObject(RO, Root, [])
RO-->>App: Promise<PathObject>
note right of DPO: Path-based operations
App->>DPO: get("users").get("alice").value()
DPO->>Root: resolve ["users","alice"]
Root-->>DPO: Value | LiveMap | LiveCounter
DPO-->>App: value | undefined
App->>DPO: at("counters.total").increment(2)
DPO->>Root: resolve to LiveCounterDeprecated
DPO->>RO: submit counter update (+2)
RO-->>App: Promise<void>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
VeskeR
force-pushed
the
PUB-2057/path-object-access-mutation
branch
from
dcc9742 to
f8eccb0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2091/features
github-actions
bot
temporarily deployed
to
staging/pull/2091/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2091/bundle-report
VeskeR
force-pushed
the
liveobjects/entry-point-docs-change
branch
from
e56a7b3 to
eecc59b
Compare
VeskeR
force-pushed
the
PUB-2057/path-object-access-mutation
branch
from
f8eccb0 to
fec26d3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2091/features
github-actions
bot
temporarily deployed
to
staging/pull/2091/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2091/typedoc
VeskeR
force-pushed
the
PUB-2057/path-object-access-mutation
branch
from
fec26d3 to
bf59a1b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2091/features
github-actions
bot
temporarily deployed
to
staging/pull/2091/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2091/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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably.d.ts (1)
2898-2950: BridgeLive*Deprecatedtypes into the newValueunion.Right now
Valueis defined in terms of the new brandedLiveMap/LiveCounter, butLiveMapType(includingAblyDefaultObject) is still expressed with the deprecated surface (LiveMapDeprecated,LiveCounterDeprecated). BecauseLiveMapDeprecatedandLiveCounterDeprecateddon’t extend the new branded counterparts,AblyDefaultObjectis not assignable toRecord<string, Value>, so even the canonical callrealtimeObject.getPathObject<AblyDefaultObject>()is rejected by the compiler. The same incompatibility ripples intoLiveMapOperations.set(...),remove(...), etc., making the new PathObject API unusable with the existing object graph.Please make the deprecated types structurally satisfy the new branded ones—e.g. have
LiveMapDeprecated<T>extendLiveMap<T>andLiveCounterDeprecatedextendLiveCounter(or otherwise include the branded marker). Once that bridge exists, the legacy typedefs become assignable toValueagain and the new API type-checks in real projects.-export declare interface LiveMapDeprecated<T extends LiveMapType> extends LiveObjectDeprecated<LiveMapUpdate<T>> { +export declare interface LiveMapDeprecated<T extends LiveMapType> + extends LiveObjectDeprecated<LiveMapUpdate<T>>, + LiveMap<T> { @@ -export declare interface LiveCounterDeprecated extends LiveObjectDeprecated<LiveCounterUpdate> { +export declare interface LiveCounterDeprecated + extends LiveObjectDeprecated<LiveCounterUpdate>, + LiveCounter {
🧹 Nitpick comments (1)
test/realtime/objects.test.js (1)
452-475: Cache the child PathObject/value before the assertionsWe’re repeatedly re-resolving the same child path (and its value) in each branch. Stashing it once tightens the helper and avoids extra work.
diff @@ - if (keyData.data.bytes != null) { + const childPath = pathObject.get(key); + const childValue = childPath.value(); + if (keyData.data.bytes != null) { helper.recordPrivateApi('call.BufferUtils.base64Decode'); helper.recordPrivateApi('call.BufferUtils.areBuffersEqual'); expect( - BufferUtils.areBuffersEqual(pathObject.get(key).value(), BufferUtils.base64Decode(keyData.data.bytes)), + BufferUtils.areBuffersEqual(childValue, BufferUtils.base64Decode(keyData.data.bytes)), msg, ).to.be.true; - expect(BufferUtils.areBuffersEqual(pathObject.get(key).value(), mapObj.get(key)), compareMsg).to.be.true; + expect(BufferUtils.areBuffersEqual(childValue, mapObj.get(key)), compareMsg).to.be.true; } else if (keyData.data.json != null) { const expectedObject = JSON.parse(keyData.data.json); - expect(pathObject.get(key).value()).to.deep.equal(expectedObject, msg); - expect(pathObject.get(key).value()).to.deep.equal(mapObj.get(key), compareMsg); + expect(childValue).to.deep.equal(expectedObject, msg); + expect(childValue).to.deep.equal(mapObj.get(key), compareMsg); } else { const expectedValue = keyData.data.string ?? keyData.data.number ?? keyData.data.boolean; - expect(pathObject.get(key).value()).to.equal(expectedValue, msg); - expect(pathObject.get(key).value()).to.equal(mapObj.get(key), compareMsg); + expect(childValue).to.equal(expectedValue, msg); + expect(childValue).to.equal(mapObj.get(key), compareMsg); }
📜 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(22 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/realtimeobject.ts(2 hunks)test/package/browser/template/src/index-objects.ts(5 hunks)test/realtime/objects.test.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
test/realtime/objects.test.js (2)
src/plugins/objects/pathobject.ts (4)
value(70-103)keys(129-133)entries(108-124)values(138-142)src/plugins/objects/livemap.ts (3)
keys(336-340)entries(321-334)values(342-346)
src/plugins/objects/pathobject.ts (1)
ably.d.ts (7)
Value(2451-2451)AnyPathObject(2616-2638)LiveMap(2428-2431)PathObject(2645-2651)Primitive(2406-2414)LiveObject(2444-2444)LiveCounter(2436-2439)
src/plugins/objects/realtimeobject.ts (3)
ably.d.ts (4)
Value(2451-2451)PathObject(2645-2651)LiveMap(2428-2431)ObjectsEvent(2269-2269)src/plugins/objects/pathobject.ts (1)
DefaultPathObject(13-249)src/plugins/objects/objectspool.ts (1)
ROOT_OBJECT_ID(9-9)
ably.d.ts (5)
src/plugins/objects/livemap.ts (3)
LiveMap(48-929)entries(321-334)LiveMapUpdate(43-45)src/plugins/objects/pathobject.ts (3)
entries(108-124)path(31-34)value(70-103)src/plugins/objects/batchcontextlivemap.ts (2)
entries(31-35)BatchContextLiveMap(7-62)src/plugins/objects/livecounter.ts (3)
LiveCounter(22-367)value(134-137)LiveCounterUpdate(17-19)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(14-18)
⏰ 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). (8)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: lint
- GitHub Check: test-npm-package
🔇 Additional comments (3)
scripts/moduleReport.ts (1)
340-343: Nice catch addingpathobject.tsto the allowlistThis keeps the bundle guardrail aligned with the new PathObject implementation so the size check won’t regress. 👍
test/realtime/objects.test.js (1)
3995-4515: Huge 👍 for the comprehensive PathObject scenariosThe breadth of cases (path resolution, collection APIs, counters, error handling, etc.) gives a lot of confidence in the new abstraction.
test/package/browser/template/src/index-objects.ts (1)
46-47: Type migration assertion looks solidNice to see the sample aligning with the deprecated surface; forcing
channel.object.get()through aLiveMapDeprecated<MyCustomObject>keeps the type contract under test and mirrors the new public API. No issues spotted here.
mschristensen
left a comment
mschristensen
left a comment
There was a problem hiding this comment.
Looks great, thanks. Some small comments
src/plugins/objects/pathobject.ts
Outdated
| } | ||
|
|
||
| /** | ||
| * Returns the fully-qualified string path that this PathObject represents |
There was a problem hiding this comment.
I think worth including a note in the comment here that map keys with dots in them will be escaped
There was a problem hiding this comment.
clarified about escaped dots both here and in the public type definitions, see https://github.com/ably/ably-js/compare/fb75485f7abe73530854aef7e72c3cdef78d9744..6954bce8746912f659ed32b5ff0e7292cadee937
| // unknown type - return undefined | ||
| return undefined; |
There was a problem hiding this comment.
Should we log a warn here? I think this would indicate a library bug, so if a customer reported this warn log we would know about it
There was a problem hiding this comment.
src/plugins/objects/pathobject.ts
Outdated
| return undefined; | ||
| } | ||
| } catch (error) { | ||
| if (this._client.Utils.isErrorInfoOrPartialErrorInfo(error)) { |
There was a problem hiding this comment.
I don't see anything in this statement which determines that the error info is indeed a path resolution error. Should we add a specific error code for path resultion errors?
There was a problem hiding this comment.
And a separate error code for a path resolving to an unexpected object type, e.g. calling keys() on a path that doesn't resolve to a LiveMap
There was a problem hiding this comment.
found existing error codes that fit nicely here:
https://faqs.ably.com/error-code-92005 - path resolution has failed, no entry at path.
https://faqs.ably.com/error-code-92007 - operation not processable at path.
updated to use them in https://github.com/ably/ably-js/compare/fb75485f7abe73530854aef7e72c3cdef78d9744..6954bce8746912f659ed32b5ff0e7292cadee937.
also created https://ably.atlassian.net/browse/PUB-3305 to update the docs for those error codes to include explanation for realtime client (currently they only talk about REST API).
src/plugins/objects/pathobject.ts
Outdated
| * Returns an iterator of [key, value] pairs for LiveMap entries | ||
| */ | ||
| *entries<U extends Record<string, Value>>(): IterableIterator<[keyof U, PathObject<U[keyof U]>]> { | ||
| const resolved = this._resolvePath(this._path); |
There was a problem hiding this comment.
I think we should treat path resolution errors consistently in value() and this and other methods. I think we should either consistently throw if the path doesn't resolve, or else we should consistently catch path resolution errors and return undefined. What do you think would provide the best experience?
There was a problem hiding this comment.
changed the path resolution approach in https://github.com/ably/ably-js/compare/fb75485f7abe73530854aef7e72c3cdef78d9744..6954bce8746912f659ed32b5ff0e7292cadee937.
matches what we have discussed previously on the call. Idea in general:
- mutation methods (which usually return a promise) always throw to indicate that operation has failed and change was not applied.
- access methods silently resolve path resolution errors and return "empty" result based on the context. for
.value()and.size()this means returningundefinedas we couldn't resolve the value/size, for iterator methods we return empty iterator (as a convenience so developers don't need to check for nullish values every time they call an iterator method)
VeskeR
force-pushed
the
liveobjects/entry-point-docs-change
branch
2 times, most recently
from
7d6a3b9 to
10d08e1
Compare
VeskeR
force-pushed
the
PUB-2057/path-object-access-mutation
branch
from
bf59a1b to
fb75485
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2091/features
github-actions
bot
temporarily deployed
to
staging/pull/2091/bundle-report
Adds new path-based types to ably.d.ts. The previous LiveObject types are temporarily marked as *Deprecated in ably.d.ts until they are fully converted to the new type system in the following PRs. The `RealtimeObject` entrypoint temporarily adds a `getPathObject` method to get a PathObject instance for a root and preserves the existing `.get()` method which returns a LiveMap instance directly so that corresponding tests do not fail yet. Resolves PUB-2057
VeskeR
force-pushed
the
PUB-2057/path-object-access-mutation
branch
from
fb75485 to
6954bce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/plugins/objects/realtimeobject.ts (1)
94-110: Align getPathObject’s generic default with get() to preserve AblyObjectsTypes inferenceWithout a default, callers lose channel-specific typings. Match
get()by bounding toAPI.LiveMapTypeand defaulting toAPI.AblyDefaultObject.- async getPathObject<T extends Record<string, API.Value>>(): Promise<API.PathObject<API.LiveMap<T>>> { + async getPathObject<T extends API.LiveMapType = API.AblyDefaultObject>(): Promise<API.PathObject<API.LiveMap<T>>> {
🧹 Nitpick comments (3)
test/realtime/objects.test.js (1)
455-479: Byte/JSON/primitive parity checks between PathObject and LiveMapLooks correct. Minor nit: store
const v = pathObject.get(key).value()once per key to avoid duplicate path resolution.test/package/browser/template/src/index-objects.ts (1)
2-2: Use type‑only imports to avoid accidental runtime importsThese are only used as types. Prefer
import typeto prevent bundlers from trying to import non-existent values.-import { LiveCounterDeprecated, LiveMapDeprecated } from 'ably'; +import type { LiveCounterDeprecated, LiveMapDeprecated } from 'ably';ably.d.ts (1)
2667-2668: Update documentation references in follow-up.Several operation method docs reference
LiveObjectDeprecated.subscribe()with TODO comments to point to PathObject subscribe method:
LiveMapOperations.set()(line 2667)LiveMapOperations.remove()(line 2683)LiveCounterOperations.increment()(line 2703)AnyOperations.set()(line 2734)AnyOperations.remove()(line 2750)AnyOperations.increment()(line 2767)Track these TODOs to update the documentation once the PathObject subscribe API is implemented.
Also applies to: 2683-2684, 2703-2704, 2734-2735, 2750-2751, 2767-2768
📜 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(23 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/realtimeobject.ts(2 hunks)test/package/browser/template/src/index-objects.ts(5 hunks)test/realtime/objects.test.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/moduleReport.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/plugins/objects/realtimeobject.ts (3)
ably.d.ts (4)
Value(2434-2434)PathObject(2648-2654)LiveMap(2411-2414)ObjectsEvent(2269-2269)src/plugins/objects/pathobject.ts (1)
DefaultPathObject(13-303)src/plugins/objects/objectspool.ts (1)
ROOT_OBJECT_ID(9-9)
test/realtime/objects.test.js (4)
src/plugins/objects/pathobject.ts (4)
value(106-145)keys(177-181)entries(150-172)values(186-190)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livecounter.ts (1)
value(134-137)src/plugins/objects/livemap.ts (3)
keys(336-340)entries(321-334)values(342-346)
src/plugins/objects/pathobject.ts (1)
ably.d.ts (7)
Value(2434-2434)AnyPathObject(2619-2641)LiveMap(2411-2414)PathObject(2648-2654)Primitive(2389-2397)LiveObject(2427-2427)LiveCounter(2419-2422)
ably.d.ts (7)
src/plugins/objects/livemap.ts (3)
LiveMap(48-929)entries(321-334)LiveMapUpdate(43-45)src/plugins/objects/pathobject.ts (3)
entries(150-172)path(33-36)value(106-145)src/plugins/objects/batchcontextlivemap.ts (2)
entries(31-35)BatchContextLiveMap(7-62)src/plugins/objects/livecounter.ts (3)
LiveCounter(22-367)value(134-137)LiveCounterUpdate(17-19)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/objectmessage.ts (1)
PrimitiveObjectValue(24-24)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(14-18)
⏰ 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-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (11)
test/realtime/objects.test.js (3)
76-85: Helper improvement LGTMOptional code assertion via withCode is clean and returning the saved error is useful for callers.
3998-4071: Strong coverage for PathObject path()/get()/at() and escapingComprehensive checks for dots and escapes; matches the parser behavior in DefaultPathObject.
4586-4589: Entrypoint setup for PathObjectObtaining
entryPathObjectviarealtimeObject.getPathObject()integrates cleanly alongsideget().src/plugins/objects/pathobject.ts (3)
64-97: LGTM! Safari-compatible path parsing.The manual string parsing correctly handles escaped dots without using negative lookbehind, addressing the Safari ≤16 compatibility issue raised in previous reviews. The logic properly:
- Splits on unescaped dots
- Converts
\.to.in segments- Preserves other escape sequences like
\\Based on learnings
106-145: LGTM! Consistent error handling pattern.The error handling correctly distinguishes between read and write operations:
- Read operations (
value(),entries(),size()) catch path resolution errors (code 92005) and returnundefinedor empty iterators- Write operations (
set(),remove(),increment(),decrement()) let path resolution errors throwThis provides graceful degradation for reads while ensuring writes fail fast on invalid paths.
Also applies to: 149-172, 195-212
128-135: Leave LOG_MAJOR and undefined fallback unchanged: Unexpected types only occur on library bugs; safe fallback is preferable to throwing.ably.d.ts (5)
2385-2434: LGTM! Well-designed type system.The new non-deprecated type system is well-structured:
Primitiveproperly covers all primitive types including JSON-serializable values- The
__livetypesymbol provides nominal typing to prevent structural compatibility betweenLiveMapandLiveCounterValueas the union ofLiveObject | Primitivecorrectly represents all possible values in the systemThis foundation enables the type-safe PathObject API.
2643-2654: LGTM! Type-safe PathObject dispatch.The
PathObject<T>conditional type correctly dispatches to the appropriate PathObject variant:
- Uses tuple wrapping
[T] extends [LiveMap<infer T>]to prevent unwanted union distribution- Extracts inner type from
LiveMap<T>to provide typed access- Falls back to
AnyPathObject<T>for unknown typesThis enables compile-time type safety for path-based navigation.
2520-2538: LGTM! Type-safe LiveMapPathObject.The
LiveMapPathObject<T>interface correctly composes multiple method sets:
- Typed
get<K extends keyof T>preserves type information during navigation- Extends both collection methods and mutation operations
- Type parameter
T extends Record<string, Value>ensures map structureThe interface provides full type safety for compile-time checking of path navigation.
1649-1649: LGTM! Consistent deprecation strategy.The deprecation changes consistently rename all legacy types to
*Deprecatedvariants:
LiveMap→LiveMapDeprecatedLiveCounter→LiveCounterDeprecatedLiveObject→LiveObjectDeprecatedThis approach:
- Preserves backward compatibility with existing APIs
- Enables gradual migration to the path-based API
- Creates a clear separation between old and new type systems
The new non-deprecated types are reserved for the PathObject API surface.
Also applies to: 1661-1661, 2282-2284, 2291-2316, 2325-2340, 2798-2800, 2928-2994, 3038-3067, 3087-3137
2619-2641: LGTM! Flexible type-erased APIs.
AnyPathObjectandAnyOperationscorrectly provide all possible methods when the underlying type is unknown:
- Generic type parameters on methods (
get<T>,value<T>) allow type hints when the caller has more information- Includes union of all collection methods and operations
- Enables runtime polymorphism while maintaining type safety where possible
This is the appropriate fallback for paths where TypeScript cannot infer the concrete type.
Also applies to: 2721-2783
VeskeR
merged commit 1d52319
into
integration/objects-breaking-api
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
Adds new path-based types to ably.d.ts.
The previous LiveObject types are temporarily marked as *Deprecated in ably.d.ts until they are fully converted to the new type system in the following PRs.
The
RealtimeObjectentrypoint temporarily adds agetPathObjectmethod to get a PathObject instance for a root and preserves the existing.get()method which returns a LiveMap instance directly so that corresponding tests do not fail yet.Resolves PUB-2057
PathObject operations follow these conventions for handling path resolution failures and underlying object type mismatches:
.value()and.size(), this means returningundefined. For iterator methods (such as.entries()), this means returning an empty iterator.Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores