Move Objects types from ably.d.ts to objects.d.ts#2130
Move Objects types from ably.d.ts to objects.d.ts#2130VeskeR merged 2 commits intointegration/objects-breaking-apifrom
ably.d.ts to objects.d.ts#2130Conversation
WalkthroughThis PR extracts LiveObjects typings into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
⏰ 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)
🔇 Additional comments (2)
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 |
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)
objects.d.ts (1)
1684-1686: Fix TS2309:export = Objectsconflicts with otherexportdeclarations.This module has 43 ES-style exports (
export interface,export type,export declare, etc.) alongsideexport = Objects;at the end. TypeScript forbids mixingexport =with other named exports in the same file, as these two export styles are incompatible.Given the public API shape shown in the file's documentation (
import Objects, { LiveCounter, LiveMap } from 'ably/objects'), switching to a default export is the appropriate solution:-declare const Objects: any; - -export = Objects; +declare const Objects: any; + +export default Objects;This allows both the default export for the plugin and named exports for types. Verify that consumers rely on
esModuleInteroporallowSyntheticDefaultImportsbefore merging.
🧹 Nitpick comments (1)
objects.d.ts (1)
61-112: Consider re‑exportingRealtimeObjectfromably.d.tsfor documentation tooling.
RealtimeChannel.objectnow uses theRealtimeObjecttype defined inobjects.d.ts, and the API Reference pipeline warns thatRealtimeObjectis referenced but not documented. If your docs generator only scansably.d.ts, you may want to re‑export an alias there (for example,export type RealtimeObject = ObjectsRealtimeObject;) so it appears in the main API docs surface.Not required for correctness, but it would clear the warning and make the type easier to discover in generated docs.
Also applies to: 2462-2464
📜 Review details
Configuration used: Organization 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(1 hunks)objects.d.ts(2 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(1 hunks)src/plugins/objects/livecountervaluetype.ts(3 hunks)src/plugins/objects/livemap.ts(9 hunks)src/plugins/objects/livemapvaluetype.ts(5 hunks)src/plugins/objects/objectmessage.ts(6 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(4 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)test/package/browser/template/src/index-objects.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/plugins/objects/livecountervaluetype.ts (3)
src/plugins/objects/index.ts (1)
LiveCounterValueType(7-7)objects.d.ts (2)
LiveCounter(168-171)LiveCounter(1653-1663)test/realtime/objects.test.js (1)
LiveCounter(19-19)
src/plugins/objects/objectmessage.ts (1)
objects.d.ts (7)
ObjectOperationAction(1472-1478)ObjectsMapSemantics(1493-1493)Primitive(118-126)ObjectOperation(1550-1569)ObjectData(1624-1629)JsonObject(146-146)JsonArray(141-141)
src/plugins/objects/realtimeobject.ts (3)
scripts/processPrivateApiData/dto.ts (1)
Record(53-53)objects.d.ts (4)
Value(183-183)PathObject(532-538)LiveMap(160-163)LiveMap(1634-1648)ably.d.ts (1)
ChannelState(50-57)
test/package/browser/template/src/index-objects.ts (3)
objects.d.ts (7)
PathObject(532-538)LiveMap(160-163)LiveMap(1634-1648)LiveCounterPathObject(374-399)LiveMapPathObject(333-369)AnyPathObject(477-523)ObjectMessage(1498-1545)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(357-461)src/plugins/objects/index.ts (1)
ObjectMessage(9-9)
src/plugins/objects/livemap.ts (1)
objects.d.ts (6)
Primitive(118-126)Value(183-183)LiveMap(160-163)LiveMap(1634-1648)LiveObject(176-176)CompactedValue(189-214)
src/plugins/objects/livemapvaluetype.ts (3)
src/plugins/objects/index.ts (1)
LiveMapValueType(8-8)objects.d.ts (2)
Value(183-183)Primitive(118-126)src/plugins/objects/livemap.ts (1)
ValueObjectData(24-27)
objects.d.ts (3)
src/plugins/objects/livecounter.ts (2)
LiveCounter(15-301)value(64-67)src/plugins/objects/pathobject.ts (2)
path(47-50)value(143-182)src/plugins/objects/objectmessage.ts (7)
ObjectMessage(357-461)ObjectOperation(130-165)ObjectsMapOp(70-75)ObjectsCounterOp(81-84)ObjectsMap(110-115)ObjectsCounter(121-124)ObjectData(39-44)
🪛 Biome (2.1.2)
objects.d.ts
[error] 1653-1653: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Actions: API Reference
objects.d.ts
[warning] 1-1: RealtimeObject is referenced by ably.RealtimeChannel.object but not included in the documentation.
🪛 GitHub Actions: Lint
objects.d.ts
[error] 1686-1686: TS2309: An export assignment cannot be used in a module with other exported elements.
🪛 GitHub Actions: Test NPM package
objects.d.ts
[error] 1686-1686: TypeScript compile error: TS2309: An export assignment cannot be used in a module with other exported elements. (during 'npm run typecheck')
⏰ 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 (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (29)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
2-3: ✓ Import restructuring aligns perfectly with PR objectives.The split correctly separates core Ably types (
EventCallback,Subscription) from Objects-specific types (PathObjectSubscriptionEvent,PathObjectSubscriptionOptions), moving them to their respective modules. All imported types are consistently used throughout the file with no functionality impact.src/plugins/objects/batchcontext.ts (1)
2-2: Type import switch to../../../objectsis consistent with the new Objects typings.The moved type-only import correctly targets the consolidated Objects type surface and does not affect runtime behavior.
ably.d.ts (1)
2277-2280: WiringRealtimeChannel.objecttoRealtimeObjectfrom./objectslooks correct.Importing
RealtimeObjectfrom the newobjects.d.tsmodule and using it forRealtimeChannel.objectcleanly decouples the Objects typings from the mainably.d.tswhile keeping the channel API surface intact.Also applies to: 2462-2464
src/plugins/objects/rootbatchcontext.ts (1)
1-1: UpdatedInstance/Valuetype import correctly targets the new Objects module.Switching the type import to
../../../objectsaligns RootBatchContext with the consolidated Objects typings and keeps the existing constructor and helper methods type‑safe.src/plugins/objects/instance.ts (1)
2-3: Type imports cleanly split between core (ably) and Objects (objects) modules.Pulling
EventCallback/Subscriptionfrom../../../ablyand the rest of the Object/Instance types from../../../objectsmatches the new type layout and keeps DefaultInstance’s signatures in sync with the public typings, without affecting runtime behavior.Also applies to: 4-13
src/plugins/objects/pathobject.ts (1)
2-15: LGTM! Clean import reorganization.The type imports are correctly split between core Ably types (
EventCallback,Subscription) and Objects-specific types, with proper use ofimport typefor type-only imports.src/plugins/objects/objectmessage.ts (4)
5-16: LGTM! Consistent type namespace migration.All type references have been correctly updated from the generic
APInamespace to theObjectsApinamespace, maintaining type safety throughout the file.
339-350: LGTM! Return type correctly updated.The
stringifyOperationfunction's return type has been properly updated to useObjectsApi.ObjectOperation, consistent with the broader namespace migration.
446-460: LGTM! Public API return type correctly updated.The
toUserFacingMessagemethod now correctly returnsObjectsApi.ObjectMessage, aligning with the new public Objects API surface.
791-794: LGTM! JSON decoding types correctly updated.The decoded JSON types now correctly reference
ObjectsApi.JsonObjectandObjectsApi.JsonArray, maintaining consistency with the new type structure.src/plugins/objects/realtimeobject.ts (3)
4-5: LGTM! Clean separation of type imports.Core Ably types (
ChannelState) are correctly imported from../../../ably, while Objects-specific types are imported from../../../objectsasObjectsApi.
83-96: LGTM! Public API method signature correctly updated.The
get()method signature now usesObjectsApi.Value,ObjectsApi.PathObject, andObjectsApi.LiveMap, properly reflecting the new Objects public API surface.
227-240: LGTM! Parameter type correctly updated.The
actOnChannelStatemethod now correctly usesChannelStatetype imported from the ably core module.src/plugins/objects/livecountervaluetype.ts (3)
1-1: LGTM! Clean import consolidation.The
__livetypesymbol andLiveCounterinterface are now correctly imported from../../../objects, consolidating Objects-related types in one module.
20-23: LGTM! Type branding correctly updated.The class now correctly implements
LiveCounterand uses the__livetypesymbol imported from the objects module, maintaining proper type discrimination.
30-37: LGTM! Factory method return type correctly updated.The
create()static method now returnsLiveCountertype from the objects module, aligning with the new public API surface.src/plugins/objects/livemapvaluetype.ts (4)
1-1: LGTM! Comprehensive type imports.All Objects-related types (
__livetype,Primitive,Value,LiveMap) are now imported from../../../objects, with a sensible alias (PublicLiveMap) to avoid naming conflicts with the runtimeLiveMapclass.
33-41: LGTM! Class declaration correctly updated.The class correctly implements
PublicLiveMap<T>and uses the__livetypesymbol from the objects module, maintaining proper type branding for external consumers.
43-52: LGTM! Factory method signature correctly updated.The
create()method signature now usesValueconstraints and returnsPublicLiveMap, properly aligning with the new Objects type system.
113-146: LGTM! Internal value handling correctly updated.The
_createInitialValueOperationmethod now usesRecord<string, Value>parameters and correctly casts primitive values toPrimitivetype, maintaining type consistency throughout the value construction process.test/package/browser/template/src/index-objects.ts (3)
1-11: LGTM! Demonstrates the improved developer experience.The new import structure clearly shows the benefit of this refactoring: developers can now import all Objects-related types from
'ably/objects'in a single statement, rather than splitting imports between'ably'and'ably/objects'. This consolidation simplifies the API surface and improves discoverability.
39-45: LGTM! Clean usage of the new API.The code now uses
RealtimeandPathObject<LiveMap<MyCustomObject>>directly without theAbly.namespace prefix, demonstrating the cleaner API that results from this consolidation.
57-71: LGTM! Type references correctly updated throughout.All type references (
LiveCounterPathObject,LiveMapPathObject,AnyPathObject,ObjectMessage) now use the imported types from'ably/objects'without namespace qualification, maintaining consistency throughout the test file.src/plugins/objects/livemap.ts (6)
3-3: LGTM! Comprehensive type imports with clear aliasing.All Objects-related types are imported from
../../../objects, withLiveObject as PublicLiveObjectproviding a clear alias to distinguish the public interface from the internal implementation class.
24-30: LGTM! Data structure types correctly updated.The
ValueObjectDataandLiveMapObjectDatainterfaces now usePrimitiveandValuetypes from the objects module, maintaining consistency with the public API surface.
48-52: LGTM! Class declaration correctly updated.The
LiveMapclass signature and branding symbol have been properly updated to useValueconstraints and the__livetypesymbol from the objects module.
87-118: LGTM! Value handling correctly updated.The
createMapSetMessagemethod now correctly usesValuetype for parameters and appropriately casts primitive values toPrimitivewhen constructingValueObjectData.
500-536: LGTM! Compact method return type correctly updated.The
compact()method now correctly returnsCompactedValue<LiveMap<T>>using types from the objects module, maintaining type consistency for the public API.
914-934: LGTM! Value resolution correctly typed.The
_getResolvedValueFromObjectDatamethod now correctly returnsValue | undefinedand casts object references toPublicLiveObject, properly distinguishing between internal implementation types and public API types.
35e48e0 to
da73ea9
Compare
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)
objects.d.ts (1)
1879-1881: Replaceexport = Objectswith an ES-style default export (fixes TS2309).The combination of
export = Objects;with numerous otherexportdeclarations in this module causesTS2309: An export assignment cannot be used in a module with other exported elements. This is blocking both lint andtsc --noEmit.You can keep a default export for
'ably/objects'while satisfying the compiler by switching to an ES-style default export:Suggested change for the module export
/** * Provides a {@link RealtimeClient} instance with the ability to use Objects functionality. * @@ * const realtime = new BaseRealtime({ ...options, plugins: { WebSocketTransport, FetchRequest, Objects } }); * ``` */ declare const Objects: any; - -export = Objects; +export default Objects;This preserves
import Objects from 'ably/objects';while removing the invalidexport =in a mixed-export module. Given this is onintegration/objects-breaking-api, the change in module typing is acceptable as part of the breaking-surface work.
♻️ Duplicate comments (1)
objects.d.ts (1)
160-171: Fix unsafe LiveMap/LiveCounter interface–class merging (blocks lint).You currently declare both
interface LiveMap/interface LiveCounter(lines 160–171) andexport class LiveMap/export class LiveCounter(lines 1826–1858) with the same names, which triggers Biome’snoUnsafeDeclarationMergingand is brittle: the compiler does not verify that the classes satisfy the branded interface members (e.g.[__livetype]).Given these are nominal value types with a static
create(...)factory, you can avoid class–interface merging entirely by:
- Keeping the interfaces as the branded instance types.
- Replacing the
export class ...declarations with value-sidedeclare constobjects exposing the staticcreatemethods.This preserves the
LiveMap.create(...)/LiveCounter.create(...)public API while removing unsafe merging and aligning better with how the runtime actually uses value types.Suggested change for LiveMap & LiveCounter declarations
@@ export interface LiveMap<_T extends Record<string, Value> = Record<string, Value>> { /** LiveMap type symbol */ [__livetype]: 'LiveMap'; } @@ -/** - * Static utilities related to LiveMap instances. - */ -export class LiveMap { - /** - * Creates a {@link LiveMap} value type that can be passed to mutation methods - * (such as {@link LiveMapOperations.set}) to assign a new LiveMap to the channel object. - * - * @param initialEntries - Optional initial entries for the new LiveMap object. - * @returns A {@link LiveMap} value type representing the initial state of the new LiveMap. - * @experimental - */ - static create<T extends Record<string, Value>>( - // block TypeScript from inferring T from the initialEntries argument, so instead it is inferred - // from the contextual type in a LiveMap.set call - initialEntries?: NoInfer<T>, - ): LiveMap<T extends Record<string, Value> ? T : {}>; -} +/** + * Static utilities related to LiveMap instances. + * + * Exposed as a value with a {@link LiveMap.create} factory function instead of a class, + * to avoid unsafe interface–class declaration merging while preserving the + * `LiveMap.create(...)` call site. + */ +export declare const LiveMap: { + /** + * Creates a {@link LiveMap} value type that can be passed to mutation methods + * (such as {@link LiveMapOperations.set}) to assign a new LiveMap to the channel object. + * + * @param initialEntries - Optional initial entries for the new LiveMap object. + * @returns A {@link LiveMap} value type representing the initial state of the new LiveMap. + * @experimental + */ + create<T extends Record<string, Value>>( + // block TypeScript from inferring T from the initialEntries argument, so instead it is inferred + // from the contextual type in a LiveMap.set call + initialEntries?: NoInfer<T>, + ): LiveMap<T extends Record<string, Value> ? T : {}>; +}; @@ export interface LiveCounter { /** LiveCounter type symbol */ [__livetype]: 'LiveCounter'; } @@ -/** - * Static utilities related to LiveCounter instances. - */ -export class LiveCounter { - /** - * Creates a {@link LiveCounter} value type that can be passed to mutation methods - * (such as {@link LiveMapOperations.set}) to assign a new LiveCounter to the channel object. - * - * @param initialCount - Optional initial count for the new LiveCounter object. - * @returns A {@link LiveCounter} value type representing the initial state of the new LiveCounter. - * @experimental - */ - static create(initialCount?: number): LiveCounter; -} +/** + * Static utilities related to LiveCounter instances. + * + * Exposed as a value with a {@link LiveCounter.create} factory function instead of a class, + * to avoid unsafe interface–class declaration merging while preserving the + * `LiveCounter.create(...)` call site. + */ +export declare const LiveCounter: { + /** + * Creates a {@link LiveCounter} value type that can be passed to mutation methods + * (such as {@link LiveMapOperations.set}) to assign a new LiveCounter to the channel object. + * + * @param initialCount - Optional initial count for the new LiveCounter object. + * @returns A {@link LiveCounter} value type representing the initial state of the new LiveCounter. + * @experimental + */ + create(initialCount?: number): LiveCounter; +};Also applies to: 1826-1858
📜 Review details
Configuration used: Organization 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(1 hunks)objects.d.ts(2 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(1 hunks)src/plugins/objects/livecountervaluetype.ts(3 hunks)src/plugins/objects/livemap.ts(10 hunks)src/plugins/objects/livemapvaluetype.ts(5 hunks)src/plugins/objects/objectmessage.ts(6 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(4 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)test/package/browser/template/src/index-objects.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/objects/rootbatchcontext.ts
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/pathobject.ts
- src/plugins/objects/objectmessage.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugins/objects/livecountervaluetype.ts (2)
src/plugins/objects/index.ts (1)
LiveCounterValueType(7-7)objects.d.ts (2)
LiveCounter(168-171)LiveCounter(1848-1858)
test/package/browser/template/src/index-objects.ts (1)
objects.d.ts (7)
PathObject(619-625)LiveMap(160-163)LiveMap(1829-1843)LiveCounterPathObject(422-459)LiveMapPathObject(366-417)AnyPathObject(547-610)ObjectMessage(1693-1740)
src/plugins/objects/livemap.ts (1)
objects.d.ts (7)
Primitive(118-126)Value(183-183)LiveMap(160-163)LiveMap(1829-1843)LiveObject(176-176)CompactedValue(189-205)CompactedJsonValue(222-247)
🪛 Biome (2.1.2)
objects.d.ts
[error] 1848-1848: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Actions: API Reference
objects.d.ts
[warning] 1-1: RealtimeObject is referenced by ably.RealtimeChannel.object but not included in the documentation.
🪛 GitHub Actions: Lint
objects.d.ts
[error] 1881-1881: TS2309: An export assignment cannot be used in a module with other exported elements.
[error] 1-1: Command failed: npx tsc --noEmit ably.d.ts modular.d.ts
🪛 GitHub Actions: Test NPM package
objects.d.ts
[error] 1881-1881: TS2309: An export assignment cannot be used in a module with other exported elements.
⏰ 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-browser (chromium)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
🔇 Additional comments (12)
ably.d.ts (1)
2277-2279: RealtimeChannel.object correctly references external RealtimeObject typings; check API docs integration.Importing
RealtimeObjectfrom./objectsand using it forRealtimeChannel.objectaligns the core d.ts with the new standalone Objects typings and fixes type reuse at call sites.The API Reference pipeline warning (“RealtimeObject is referenced by ably.RealtimeChannel.object but not included in the documentation”) likely means the docs generator isn’t yet including
objects.d.ts. You’ll want to confirm that RealtimeObject and the rest of the Objects surface are wired into the docs build (or add a forwarding doc stub if that’s how the tooling works).Also applies to: 2462-2464
src/plugins/objects/batchcontext.ts (1)
2-10: Type import source switch to../../../objectslooks correct.The move of
AnyBatchContext,BatchContext,CompactedJsonValue,CompactedValue,Instance,Primitive, andValueto import from../../../objectsis consistent with the consolidation of Objects typings and does not affect runtime behavior.src/plugins/objects/instance.ts (1)
2-4: Imports realigned between core and objects typings; implementation stays consistent.Pulling
EventCallback/Subscriptionfrom../../../ablyand all Objects model types from../../../objectscorrectly mirrors the new public type layout.DefaultInstance’s signatures (subscribe, batch, etc.) still match the corresponding interfaces inobjects.d.ts.Also applies to: 14-14
src/plugins/objects/livecountervaluetype.ts (1)
1-4: LiveCounter value type correctly bound to new branded interface.
LiveCounterValueTypenow implements theLiveCounterinterface from../../../objectsand uses the shared__livetypesymbol, so instances are properly branded while callers continue to useLiveCounter.create(...)via the public surface. The constructor remains private and immutable, so runtime semantics are preserved.Also applies to: 20-37
src/plugins/objects/realtimeobject.ts (1)
4-6: RealtimeObject now correctly typed against shared objects/core enums.
get<T extends Record<string, ObjectsApi.Value>>()returningPromise<ObjectsApi.PathObject<ObjectsApi.LiveMap<T>>>lines up with the publicRealtimeObjectdefinition and ensures callers get the right PathObject/LiveMap typing.- Swapping to the shared
ChannelStatetype foractOnChannelStateand_throwIfInChannelStatekeeps state handling consistent withRealtimeChannel.stateand tightens the checks inthrowIfInvalid*ApiConfiguration.No functional changes, just safer and better-aligned typings.
Also applies to: 83-96, 227-240, 478-482
src/plugins/objects/livemapvaluetype.ts (1)
1-2: LiveMap value type correctly migrated to new Value/Primitive/LiveMap interfaces.The changes to:
- Implement
PublicLiveMap<T>with the shared__livetypesymbol,- Use
Value/Primitivefrom../../../objectsin generics and in_createInitialValueOperation, and- Return
PublicLiveMap<T extends Record<string, Value> ? T : {}>fromcreateensure this value type matches the consolidated public typings while preserving existing behavior, including nested map/counter creation.
Also applies to: 33-52, 113-141
src/plugins/objects/livemap.ts (6)
3-11: LGTM! Imports properly aliased to avoid naming collisions.The imports correctly bring in the types from the
objectsmodule with appropriate aliasing (PublicLiveMap,PublicLiveObject) to prevent conflicts with the class names defined in this file.
32-35: LGTM! Type correctly updated to use local import.The
Primitivetype is now correctly imported from theobjectsmodule instead of using theAPI.Primitivereference.
50-50: LGTM! Type references consistently updated throughout.The systematic replacement of
API.ValuewithValueandAPI.PrimitivewithPrimitiveis correctly applied across interface definitions, method signatures, and type assertions.Also applies to: 95-100, 123-126, 174-174
509-509: LGTM! Return type correctly updated to use imported types.The
compactmethod return type now correctly referencesCompactedValue<PublicLiveMap<T>>using the imported types from theobjectsmodule.
551-551: LGTM! Return type correctly updated consistently withcompactmethod.The
compactJsonmethod return type is correctly updated to useCompactedJsonValue<PublicLiveMap<T>>, maintaining consistency with thecompactmethod.
965-985: LGTM! Internal method types correctly updated.The
_getResolvedValueFromObjectDatamethod's return type and internal cast are correctly updated to useValueandPublicLiveObjectfrom the imported types.
da73ea9 to
c612a1e
Compare
c612a1e to
c085efe
Compare
c085efe to
98eae84
Compare
a15e8ea to
f73a689
Compare
98eae84 to
82a9c5d
Compare
f73a689 to
7c93055
Compare
82a9c5d to
5d91782
Compare
5d91782 to
e59b36a
Compare
e59b36a to
ce47cdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
objects.d.ts (1)
1848-1858: [Previously flagged] Unsafe interface-class declaration merging for LiveCounter.This issue was already identified in a previous review. The class
LiveCounterat line 1848 merges with the interfaceLiveCounterat lines 168-171, which is unsafe because TypeScript doesn't verify that the interface's branded[__livetype]property is properly initialized in the class.The same issue exists for
LiveMap(interface at lines 160-163, class at lines 1829-1843).As previously suggested, consider replacing these classes with namespaces to eliminate the merging risk while preserving the static
.create()API.
📜 Review details
Configuration used: Organization 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 (15)
ably.d.ts(1 hunks)objects.d.ts(2 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(1 hunks)src/plugins/objects/livecounter.ts(2 hunks)src/plugins/objects/livecountervaluetype.ts(3 hunks)src/plugins/objects/livemap.ts(10 hunks)src/plugins/objects/livemapvaluetype.ts(5 hunks)src/plugins/objects/objectmessage.ts(6 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(4 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)test/package/browser/template/src/index-objects.ts(4 hunks)typedoc.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/objectmessage.ts
- src/plugins/objects/instance.ts
- src/plugins/objects/batchcontext.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/objects/livecounter.ts (2)
objects.d.ts (3)
LiveCounter(168-171)LiveCounter(1848-1858)LiveObject(176-176)test/realtime/objects.test.js (1)
LiveCounter(19-19)
src/plugins/objects/livemapvaluetype.ts (3)
src/plugins/objects/index.ts (1)
LiveMapValueType(8-8)objects.d.ts (2)
Value(183-183)Primitive(118-126)src/plugins/objects/livemap.ts (1)
ValueObjectData(32-35)
test/package/browser/template/src/index-objects.ts (1)
objects.d.ts (7)
PathObject(619-625)LiveMap(160-163)LiveMap(1829-1843)LiveCounterPathObject(422-459)LiveMapPathObject(366-417)AnyPathObject(547-610)ObjectMessage(1693-1740)
src/plugins/objects/livecountervaluetype.ts (3)
src/plugins/objects/index.ts (1)
LiveCounterValueType(7-7)objects.d.ts (2)
LiveCounter(168-171)LiveCounter(1848-1858)src/plugins/objects/livecounter.ts (1)
LiveCounter(16-304)
objects.d.ts (2)
src/plugins/objects/pathobject.ts (2)
path(48-51)value(173-212)src/plugins/objects/instance.ts (1)
value(97-125)
🪛 Biome (2.1.2)
objects.d.ts
[error] 1848-1848: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Actions: Lint
typedoc.json
[error] 1-1: Prettier formatting check reported issues in 'typedoc.json'. Run 'prettier --write' to fix.
🪛 GitHub Actions: Test NPM package
test/package/browser/template/src/index-objects.ts
[error] 46-46: TS2719: Type 'import(".../test/package/browser/build/node_modules/ably/objects").LiveMapPathObject' is not assignable to type 'import(".../test/package/browser/build/node_modules/ably/objects").LiveMapPathObject'. Two different types with this name exist, but they are unrelated.
⏰ 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 (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
🔇 Additional comments (4)
src/plugins/objects/livemapvaluetype.ts (1)
1-160: LGTM! Type imports successfully migrated from API namespace.The refactoring consistently updates all type references from the API namespace to the objects module. The imports, class declaration, method signatures, and type assertions are all properly aligned with the new type location.
ably.d.ts (1)
2277-2279: LGTM! Clean delegation to external module.The import statement and comment correctly indicate that LiveObjects types are now consolidated in
objects.d.ts. The import path resolves properly,RealtimeObjectis exported from the objects module, and no orphaned type definitions remain in ably.d.ts. The type surface consolidation is complete.test/package/browser/template/src/index-objects.ts (2)
1-12: The imports are correctly configured. Theably/objectsmodule is a dedicated export in package.json with its own type definitions (objects.d.ts), andably.d.tsintentionally does not re-export these types. There are no conflicting module exports or duplicate type definitions—the types are cleanly separated by design. No further changes needed.Likely an incorrect or invalid review comment.
46-46: Investigate circular dependency between type definitions causing TS2719 error.The TS2719 error occurs due to a circular dependency between
ably.d.tsandobjects.d.ts, not duplicate type definitions:
ably.d.ts(line 2279) importsRealtimeObjectfrom./objectsobjects.d.ts(line 10) imports types (ErrorInfo,EventCallback,RealtimeClient,StatusSubscription,Subscription) from./ablyThis circular dependency causes TypeScript to instantiate the same types through multiple import paths, resulting in the compiler treating them as unrelated types. Breaking this circular dependency by restructuring the imports (e.g., moving type dependencies to a shared types file or making
objects.d.tsimports conditional) would resolve the issue.
Before this change, developers needed to import Objects types from both
'ably' (Objects interfaces) and 'ably/objects' (LiveMap and LiveCounter
classes with static `.create()` methods):
```javascript
import * as Ably from 'ably';
import Objects { LiveCounter, LiveMap } from 'ably/objects';
type Project = {
score: Ably.LiveCounter; // type from 'ably'
};
await project.set('score', LiveCounter.create(0)); // class from 'ably/objects'
```
Now all Objects types are consolidated in 'ably/objects':
import { Objects, LiveCounter, LiveMap } from 'ably/objects';
This change also required solving two TypeScript module resolution
issues:
1. The __livetype unique symbol (used to brand LiveMap/LiveCounter
interfaces) was now being declared separately in both objects.d.ts and
objects.d.mts after the changes made in [1] to add support for named
exports. Since unique symbols are only equal to themselves, TypeScript
saw branded types from these files as incompatible.
2. When ably.d.ts imports from './objects', TypeScript resolves to
objects.d.ts. But ESM users importing from 'ably/objects' get
objects.d.mts. This caused RealtimeChannel.object to return types
from a different module than what users import and cause type mismatches.
Solution:
- Export __livetype from ably.d.ts so both objects type files import
and share the same symbol.
- Remove RealtimeObject import from ably.d.ts and use module
augmentation in objects.d.ts to add the object property back to
RealtimeChannel for projects where users import from 'ably/objects',
ensuring all Objects types come from the same module users import from.
Alternatives considered:
- Keep types in ably.d.ts with re-exports from objects.d.ts: defeats
the purpose of consolidating imports to 'ably/objects' export
- Create separate ably.d.mts for ESM: cleaner solution but requires
additional build infrastructure. This is the ideal solution and the one
we will most likely implement in the future as part of the general
ably-js type improvements
[1] #2131
ce47cdc to
3dbf57a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
objects.d.ts (1)
148-170: Resolve interface–class merging forLiveMap/LiveCounterto satisfy Biome.You currently have both:
export interface LiveMap<...> { [__livetype]: 'LiveMap'; }andexport class LiveMap { static create(...)... }export interface LiveCounter { [__livetype]: 'LiveCounter'; }andexport class LiveCounter { static create(...)... }which rely on TypeScript’s interface–class declaration merging. Biome flags this as unsafe (
noUnsafeDeclarationMerging), and the same concern was raised on an earlier revision of this file. The merge is only being used to hang a staticcreatehelper off the branded value type, but the lint error will continue to fail CI.To keep the public surface (
LiveMap.create(...)/LiveCounter.create(...)returning branded value types) while avoiding merging, two viable patterns are:
Fold the brand into the class and drop the interfaces
In this
.d.tsfile:
- Remove the separate
export interface LiveMapandexport interface LiveCounter.- Change the class declarations to include the branded field on the instance:
export class LiveMap<T extends Record<string, Value> = Record<string, Value>> { readonly [__livetype]: 'LiveMap'; static create<T extends Record<string, Value>>( initialEntries?: NoInfer<T>, ): LiveMap<T extends Record<string, Value> ? T : {}>; } export class LiveCounter { readonly [__livetype]: 'LiveCounter'; static create(initialCount?: number): LiveCounter; }and keep using
LiveMap/LiveCounterinValue,LiveObject, conditional types, etc. This preserves the API (LiveMap.create/LiveCounter.create) and the branded instances, without any interface–class merging.Or: switch to namespaces for the static helpers
- Keep the branded interfaces as they are.
- Replace the
class LiveMap/class LiveCounterdeclarations with:export namespace LiveMap { function create<T extends Record<string, Value>>( initialEntries?: NoInfer<T>, ): LiveMap<T extends Record<string, Value> ? T : {}>; } export namespace LiveCounter { function create(initialCount?: number): LiveCounter; }and adjust the runtime implementation to export a value with matching static functions.
Either approach removes the unsafe merge and should clear the Biome error while keeping the public API shape intact.
Also applies to: 1823-1852
🧹 Nitpick comments (2)
ably.d.ts (1)
2288-2385: Channel / annotations surfaces look coherent; consider makinggetparams optional.The new
Channel,RestAnnotations, and expandedRealtimeChanneltypings are internally consistent (history returnsPaginatedResult<InboundMessage>, annotations are split into REST vs realtime, and presence/push wiring matches existing patterns). The only minor ergonomics nit is:
RestAnnotations.getandRealtimeAnnotations.getrequire aparams: GetAnnotationsParams | nullargument instead of making it optional, which will force callers to passnullwhen they don't need filters.If you don't have a strong reason to require
null, you may want to relax those toparams?: GetAnnotationsParams | nullto better match the rest of the API surface.Also applies to: 2390-2554
objects.d.ts (1)
1881-1893: Typedoc warning about multipleobjectdeclarations is likely from this module augmentation.The module augmentation adds
RealtimeChannel.object: RealtimeObjectwith its own doc comment here, whileRealtimeChannelis documented separately inably.d.ts. Typedoc’s “object has multiple declarations with a comment; an arbitrary comment will be used” warning suggests it’s seeing two documented declarations for the same property.If you care about the generated API docs for
RealtimeChannel.object, consider consolidating the documentation into a single declaration (or making one of them undocumented) so Typedoc has a single source of truth.
📜 Review details
Configuration used: Organization 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 (15)
ably.d.ts(2 hunks)objects.d.ts(2 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/instance.ts(1 hunks)src/plugins/objects/livecounter.ts(2 hunks)src/plugins/objects/livecountervaluetype.ts(3 hunks)src/plugins/objects/livemap.ts(10 hunks)src/plugins/objects/livemapvaluetype.ts(5 hunks)src/plugins/objects/objectmessage.ts(6 hunks)src/plugins/objects/pathobject.ts(1 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(4 hunks)src/plugins/objects/rootbatchcontext.ts(1 hunks)test/package/browser/template/src/index-objects.ts(4 hunks)typedoc.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- typedoc.json
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/batchcontext.ts
- src/plugins/objects/realtimeobject.ts
- src/plugins/objects/instance.ts
- src/plugins/objects/pathobject.ts
- src/plugins/objects/objectmessage.ts
- test/package/browser/template/src/index-objects.ts
- src/plugins/objects/livecountervaluetype.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugins/objects/livemapvaluetype.ts (3)
src/plugins/objects/index.ts (1)
LiveMapValueType(8-8)objects.d.ts (2)
Value(177-177)Primitive(118-126)src/plugins/objects/livemap.ts (1)
ValueObjectData(32-35)
src/plugins/objects/livemap.ts (1)
objects.d.ts (7)
Primitive(118-126)Value(177-177)LiveMap(154-157)LiveMap(1823-1837)LiveObject(170-170)CompactedValue(183-199)CompactedJsonValue(216-241)
ably.d.ts (2)
src/common/lib/client/restchannelmixin.ts (1)
RestHistoryParams(10-15)src/common/lib/client/paginatedresource.ts (1)
PaginatedResult(134-192)
🪛 Biome (2.1.2)
objects.d.ts
[error] 1842-1842: This class is unsafely merged with an interface.
The interface is declared here.
The TypeScript compiler doesn't check whether properties defined in the interface are initialized in the class.
(lint/suspicious/noUnsafeDeclarationMerging)
🪛 GitHub Actions: API Reference
objects.d.ts
[warning] 1888-1888: object has multiple declarations with a comment. An arbitrary comment will be used.
⏰ 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). (7)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (9)
ably.d.ts (1)
302-311: __livetype branding export is consistent with the new Objects typings.Exporting a shared
__livetypeunique symbol here cleanly supports the brandedLiveMap/LiveCounterinterfaces inobjects.d.tsand the TS sources that use[__livetype]for discrimination, without leaking implementation details. No issues from a types or API-compat standpoint.src/plugins/objects/rootbatchcontext.ts (1)
1-1: SwitchingInstance/Valueimports to../../../objectsis correct and side‑effect free.This keeps the implementation aligned with the new Objects typings surface and avoids depending on the root
ablytypings for Objects-specific types. No behavior change and the types still match the usage in this file.src/plugins/objects/livecounter.ts (1)
1-2: BrandingLiveCounterwith__livetypeand implementing the public interface is sound.Importing
__livetypefromablyand havingLiveCounterimplement the publicLiveCounterinterface fromobjects, with adeclare readonly [__livetype]: 'LiveCounter'field, correctly aligns the runtime class with the branded value type used in the public typings without changing behavior.Also applies to: 17-19
src/plugins/objects/livemapvaluetype.ts (1)
1-2: LiveMapValueType now correctly targets the public Objects typings and branded Value graph.
- Constraining
Tand internalentriestoRecord<string, Value>and implementingPublicLiveMap<T>aligns this value type with the newValueunion inobjects.d.ts.- The
declare readonly [__livetype]: 'LiveMap'field plus the existing_livetyperuntime marker give you both compile‑time branding and dependable runtime detection.- Updating
_createInitialValueOperationto work withRecord<string, Value>and casting the validated leaf values toPrimitivewhen constructingValueObjectDatais consistent with the new Primitive definition and the prior validation viaLiveMap.validateKeyValue.No functional regressions apparent; the type surface matches the new public Objects module.
Also applies to: 34-36, 44-47, 114-117, 140-141
src/plugins/objects/livemap.ts (5)
2-11: Import restructuring aligns with the consolidation goal.The imports correctly separate the branding symbol (
__livetype) from the public type interfaces. The aliasing ofLiveMap as PublicLiveMapandLiveObject as PublicLiveObjectappropriately avoids naming conflicts with the class definitions in this file.
56-60: Class declaration correctly implements the public interface.The change from
implements LiveMap<T>toimplements PublicLiveMap<T>properly resolves the previous review concern. The class now correctly implements the imported interface rather than self-referencing.
95-100: Method signature correctly uses the consolidatedValuetype.The parameter type update is consistent with the type definitions in
objects.d.ts.
509-551: Return types correctly reference the public interface.The
compactandcompactJsonmethods now return types parameterized withPublicLiveMap<T>, which aligns with theCompactedValueandCompactedJsonValuegeneric definitions.
965-985: Return type and cast correctly expose the public interface.The method now returns
Value | undefined(the public type), and the cast on line 984 appropriately bridges the internalLiveObjectimplementation to thePublicLiveObjectinterface. The double cast throughunknownis a standard pattern for this scenario since the types are structurally compatible but nominally distinct.
3dbf57a to
972be63
Compare
…objects.d.ts Problem: After moving Objects types from ably.d.ts to objects.d.ts, `npm run docs` failed with: "RealtimeObject, defined in ./objects.d.ts, is referenced by ably.RealtimeChannel.object but not included in the documentation." Adding objects.d.ts as a typedoc entry point initially seemed to fix it, but after running `npm run build`, docs failed again with: "object has multiple declarations with a comment" pointing to both objects.d.ts:1888 and objects.d.mts:1888. Attempted fixes that didn't work: - typedoc's `exclude: ["objects.d.mts"]` option - typedoc's `exclude: ["**/objects.d.mts"]` glob pattern Both still resulted in typedoc finding the .mts file and failing. Root cause: Both objects.d.ts and objects.d.mts contain identical module augmentation (`declare module './ably'`) adding the `object` property to RealtimeChannel. TypeDoc's `exclude` option only prevents files from being used as entry points (or at least it seems so to me) - it doesn't prevent TypeScript from resolving them as dependencies during module resolution. As a result, TypeDoc manages to discover objects.d.mts through its module resolution algorithm. Solution: Create a dedicated tsconfig.typedoc.json that extends the base tsconfig but explicitly excludes objects.d.mts. This prevents TypeScript itself from seeing the file during typedoc's analysis, avoiding the duplicate declaration conflict.
972be63 to
3cfb8df
Compare
2e0d030
into
integration/objects-breaking-api
Move Objects types from `ably.d.ts` to `objects.d.ts`
Before this change, developers needed to import Objects types from both
'ably' (Objects interfaces) and 'ably/objects' (LiveMap and LiveCounter
classes with static
.create()methods):Now all Objects types are consolidated in 'ably/objects':
import { Objects, LiveCounter, LiveMap } from 'ably/objects';
This change also required solving two TypeScript module resolution
issues:
interfaces) was now being declared separately in both objects.d.ts and
objects.d.mts after the changes made in [1] to add support for named
exports. Since unique symbols are only equal to themselves, TypeScript
saw branded types from these files as incompatible.
objects.d.ts. But ESM users importing from 'ably/objects' get
objects.d.mts. This caused RealtimeChannel.object to return types
from a different module than what users import and cause type mismatches.
Solution:
and share the same symbol.
augmentation in objects.d.ts to add the object property back to
RealtimeChannel for projects where users import from 'ably/objects',
ensuring all Objects types come from the same module users import from.
Alternatives considered:
the purpose of consolidating imports to 'ably/objects' export
additional build infrastructure. This is the ideal solution and the one
we will most likely implement in the future as part of the general
ably-js type improvements
[1] #2131
Summary by CodeRabbit
Refactor
New Features
Chore
✏️ Tip: You can customize this high-level summary in your review settings.