[AIT-31] LiveObjects Path-based API migration guide#2124
[AIT-31] LiveObjects Path-based API migration guide#2124VeskeR merged 3 commits intointegration/objects-breaking-apifrom
Conversation
WalkthroughAdded a LiveObjects migration guide for ably-js v2.16, clarified two liveobjects type doc strings, and refactored RealtimeObject's subscription API to return Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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)
🧰 Additional context used🪛 LanguageTooldocs/migration-guides/v2/liveobjects.md[style] ~554-~554: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym. (ADVERB_REPETITION_PREMIUM) ⏰ 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)
🔇 Additional comments (1)
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
📜 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 (1)
docs/migration-guides/v2/liveobjects.md(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Lint
docs/migration-guides/v2/liveobjects.md
[warning] 1-1: Code style issues found in this file by Prettier. Run 'prettier --write' to fix.
⏰ 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 (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (9)
docs/migration-guides/v2/liveobjects.md (9)
1-50: Comprehensive and well-structured migration guide.The introduction and overview effectively set up the migration context with clear key improvements. The table of contents provides good navigability.
44-88: Entrypoint and PathObject sections are clear and thorough.The Before/After pattern effectively illustrates the API shift from
channel.objectstochannel.object, and the PathObject explanation provides good rationale for the design change (runtime resolution, resilience to replacements). The chaining examples (.get()calls) and.at()method are well-documented.
90-122: Excellent explanation of runtime resolution behavior.The breakdown of how PathObject handles non-existent paths (access methods return defaults, mutation methods throw errors) is clear and practical. This distinction helps developers understand error handling expectations without needing to read source code.
143-188: Object creation examples are clear and show progression.The transition from individual
.createCounter()/.createMap()to static factory methods (.create()) is well-illustrated. The nested structure example effectively demonstrates a key improvement: creating deeply nested objects in a single operation. This is a strong selling point for the migration.
190-238: Subscription pattern updates are thorough.The new
{ object, message }destructuring signature is clearly shown, and the depth-control feature is well-explained with practical examples. The shift from instance-based to path-based subscriptions (resilient to object replacements) is a key benefit clearly articulated.
269-351: Instance API section handles a complex transition well.The distinction between PathObject (resolves path on each call) and Instance (operates on specific object) is clearly explained. Runtime type checking behavior and the note about the old API returning explicit instances are helpful for developers accustomed to the previous version. Instance subscription auto-unsubscribe on garbage collection is a useful detail.
353-441: TypeScript-specific changes are comprehensive.The removal of global
AblyObjectsTypesand shift to explicit generic parameters (channel.object.get<T>()) is well-explained. The extended example showing nested type inference throughPathObject<LiveMap<...>>effectively demonstrates the type safety benefits. This section should help TS users maintain type safety during migration.
443-528: Common migration patterns provide practical guidance.The progression from reading → updating → observing → working with instances covers typical workflows. Each pattern shows both old and new idioms side-by-side, making it easy to map existing code to new API.
530-613: New features section effectively showcases benefits.The
.compact()method, async iterator API, and implicit channel attach are presented with clear use cases. The mention of automatic base64 encoding for binary data in compact representation is a helpful implementation detail.
| const player = myObject.get('players').get('player1').instance(); | ||
| // player is a LiveMap | undefined | ||
| if (player) { | ||
| console.log(player.id()); // map:... |
There was a problem hiding this comment.
I don't know whether this applies to any other methods (I know that some can throw errors so it makes sense for them to be methods) but id seems like it should just be a property and not a method?
There was a problem hiding this comment.
Instance object can wrap a primitive value as well as a LiveObject. In this case, id() returns undefined as this is not a valid operation on a primitive value (doesn't have an ID). As such, this was left as method
There was a problem hiding this comment.
I'm not sure I understand the connection; why does it need to be a method in order to return undefined for primitive Instances?
There was a problem hiding this comment.
Oh, sorry, I see. Yeah, technically it can be a getter that checks the type of the underlying instance. We don't really do any computations to have it as a method, but I think we just didn't consider to make it just a prop? The whole API surface for LiveObjects is methods. Even stuff like .size() is a method (but rightly so, it iterates through entries and performs additional checks whether to count them in or not, as opposed to just returning the number of entries in the internal map), so we didn't really think much of introducing properties to the API.
Do you feel like this is something we should consider for ably-js specifically, or can be left up to the specific SDK to decide how they want to expose id based on what is most idiomatic?
There was a problem hiding this comment.
I think that individual SDKs can choose, will leave it to you to decide based on what we do elsewhere in JS.
There was a problem hiding this comment.
Had a look at other types we have ,more specifically classes with behavior and unique identifiers.
Found two examples: Connection and LocalDevice describe a type that is more than just a "data structure" and expose id as a prop, not a method.
As such, changed id for liveobjects API to also be a prop (getter in this instance as we need to do some checks), see: #2128.
Updated the guide to reflect the change, see: https://github.com/ably/ably-js/compare/8a291ea8eee55ea0441dafd2245786b7afe87548..8b901e01c96df6b6a5077e05e0286424932c10e6
b083173 to
066b2cf
Compare
066b2cf to
8e7b06d
Compare
8e7b06d to
93f4d26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
docs/migration-guides/v2/liveobjects.md(1 hunks)src/plugins/objects/realtimeobject.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/objects/realtimeobject.ts (1)
ably.d.ts (3)
ObjectsEvent(2308-2308)ObjectsEventCallback(1658-1658)StatusSubscription(1702-1711)
⏰ 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 (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (1)
src/plugins/objects/realtimeobject.ts (1)
90-99: StatusSubscription return type matches implementation semantics.The change to return
API.StatusSubscriptionis consistent with the d.ts definition and the existing behavior of returning an object with anoff()function that unregisters the listener. No further changes needed here.
93f4d26 to
5c2f469
Compare
5c2f469 to
33f2371
Compare
6f46f9d to
f9c7d38
Compare
8b901e0 to
ffa3382
Compare
bf217c0 to
8a476d5
Compare
ffa3382 to
0f44ea4
Compare
8a476d5 to
dc0bdeb
Compare
0f44ea4 to
3b7324d
Compare
dc0bdeb to
a7462b1
Compare
3b7324d to
ca49792
Compare
21f8a02 to
397d740
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
docs/migration-guides/v2/liveobjects.md(1 hunks)liveobjects.d.ts(2 hunks)src/plugins/liveobjects/realtimeobject.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/liveobjects/realtimeobject.ts (2)
liveobjects.d.ts (2)
ObjectsEvent(53-53)ObjectsEventCallback(58-58)ably.d.ts (1)
StatusSubscription(1698-1707)
🪛 GitHub Actions: API Reference
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 GitHub Actions: Bundle size report
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 GitHub Actions: Lint
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 GitHub Actions: Spec coverage report
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 GitHub Actions: Test (react hooks)
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'. Build failed during npm run build.
🪛 GitHub Actions: Test browser
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 GitHub Actions: Test NodeJS
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: Cannot find namespace 'API'.
🪛 GitHub Actions: Test NPM package
src/plugins/liveobjects/realtimeobject.ts
[error] 94-94: TS2503: Cannot find namespace 'API'.
🪛 LanguageTool
docs/migration-guides/v2/liveobjects.md
[style] ~554-~554: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ... been moved from the 'ably' export to 'ably/liveobjects'. This consolidates all Li...
(ADVERB_REPETITION_PREMIUM)
🔇 Additional comments (8)
src/plugins/liveobjects/realtimeobject.ts (1)
105-114: LGTM! Good defensive programming.The
off()method implementation is solid. The nil check at lines 109-111 prevents accidentally calling.off()without arguments and removing all callbacks, which is a common pitfall with EventEmitter implementations.liveobjects.d.ts (2)
258-258: Documentation clarification looks good.The updated doc comment accurately describes that the PathObject represents "the path at which there was an object change," which aligns well with the path-based subscription model documented in the migration guide.
1439-1439: Consistent documentation update.This change aligns with the
PathObjectBase.subscribedocumentation at line 258, providing consistent terminology across the API surface.docs/migration-guides/v2/liveobjects.md (5)
134-169: Excellent explanation of runtime resolution semantics.The section clearly distinguishes between access methods (which return safe defaults) and mutation methods (which fail fast with errors). This is a critical concept for developers to understand when migrating to the PathObject API.
246-255: Important clarification about value type reuse.This note effectively prevents a common misconception - developers might expect that reusing a value type would create references to the same object, when in fact each assignment creates a distinct LiveObject. Well done including this clarification.
603-625: Helpful TypeScript generics example.The example effectively demonstrates how TypeScript's type system works with the new PathObject API, showing how types flow through nested
.get()calls and how.compact()returns properly typed objects. This will help developers leverage type safety when migrating.
848-874: Nice addition of async iterator API examples.The async iterator examples demonstrate both basic usage and depth-controlled subscriptions. The note about breaking from the loop automatically unsubscribing is a helpful detail that developers might not expect.
1-866: Comprehensive and well-structured migration guide.This migration guide effectively covers the PathObject-based API changes in v2.16. The structure is logical, progressing from conceptual understanding through breaking changes to new features. The before/after examples are particularly helpful for developers migrating their code.
A few strengths:
- Clear explanation of PathObject runtime resolution semantics
- Important notes about value type reuse and instance behavior
- Side-by-side migration patterns for common scenarios
- Good coverage of both JavaScript and TypeScript changes
Note: Past reviews have flagged Prettier formatting issues (blank lines before code blocks). Since these are automatically detected by the linter, run
prettier --write docs/migration-guides/v2/liveobjects.mdto auto-fix before merging.
397d740 to
b77e4c2
Compare
ttypic
left a comment
There was a problem hiding this comment.
Migration guide looks good to me, not sure that other changes (in the code and type declaration) should be in this PR
|
|
||
| export type ObjectsEventCallback = () => void; | ||
|
|
||
| export interface OnObjectsEventResponse { |
There was a problem hiding this comment.
should these changes be part of this PR?
There was a problem hiding this comment.
This diff specifically is an internal type duplicate and this commit just removed it from the internal codebase.
We had the same named public type that was changed to StatusSubscription, and it's being used by the codebase directly now.
And the public type change is mentioned here:
https://github.com/ably/ably-js/pull/2124/changes#diff-f16182659ccbfe39d69f5b8ce319af1a0cc65725070ec11b6cabc76a40d8b787R631
There was a problem hiding this comment.
oh, sorry, if you're asking why this change is in this PR at all - I noticed the dupe when writing the migration guide and didn't bother to open a separate PR such a small change that doesn't change anything anyway
Part of AIT-31 - docs for LiveObjects path-based API
- Add section documenting plugin import changes: path renamed from 'ably/objects' to 'ably/liveobjects', default export changed to named export, plugin name changed from Objects to LiveObjects - Add TypeScript section for types moving from 'ably' to 'ably/liveobjects' - Add section for RealtimeObject.offAll() removal with individual listener management alternatives - Update existing code examples to use 'ably/liveobjects' import path - Mention UMD bundle global is now AblyLiveObjectsPlugin
b77e4c2 to
760ddc2
Compare
Related to AIT-31
Summary by CodeRabbit
Documentation
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.