Add LiveObjects read-only public API docs (ably.d.ts types)#1928
Add LiveObjects read-only public API docs (ably.d.ts types)#1928VeskeR merged 2 commits intointegration/liveobjectsfrom
ably.d.ts types)#1928Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several new TypeScript interfaces and methods to the Ably library, enhancing its support for real-time data structures. Key additions include the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
ably.d.ts (1)
2127-2128: Typographical error in documentation comment.In the comment for
LiveObjectUpdate, the sentence should read:Holds an update object which describes changes applied to the object.
bf0ffeb to
8471516
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ably.d.ts (4)
2067-2079: Consider adding iteration methods toLiveMap.The
LiveMapinterface currently lacks methods to iterate over its entries, keys, or values. Adding methods likeentries(),keys(), andvalues()would enhance usability and align it with standard Map interfaces.Consider adding:
interface LiveMap extends LiveObject<LiveMapUpdate> { // Existing methods... get(key: string): LiveObject | StateValue | undefined; size(): number; // Suggested iteration methods entries(): IterableIterator<[string, LiveObject | StateValue]>; keys(): IterableIterator<string>; values(): IterableIterator<LiveObject | StateValue>; }
2098-2110: Clarify theLiveCounterUpdateinterface.The
LiveCounterUpdateinterface specifies anupdateproperty with anincvalue. Consider generalizing this to adeltaproperty to represent both increment and decrement operations clearly.Proposed change:
export declare interface LiveCounterUpdate extends LiveObjectUpdate { update: { /** * The value by which the counter was changed. * Positive values indicate increments; negative values indicate decrements. */ delta: number; }; }
1567-1567: Typo inLiveObjectUpdateCallbacktype definition.There's a minor typographical error in the comment for
LiveObjectUpdateCallback. The sentence "A callback used inLiveObjectto listen for updates to the Live Object." has redundant wording.Suggested correction:
- Change "listen for updates to the Live Object" to "listen for updates."
2041-2043: Typographical correction ingetRoot()method documentation.There is a redundant phrase in the documentation comment for
getRoot(). The sentence can be more concise.Suggested correction:
- Modify the description to:
@returns A promise that fulfills with a
LiveMapobject upon success or rejects with anErrorInfoobject upon failure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ably.d.ts(2 hunks)
🔇 Additional comments (3)
ably.d.ts (3)
2053-2066:
Missing methods to modify the LiveMap (e.g., set, delete, clear).
The LiveMap interface lacks methods to modify the map's contents. Without methods like set, delete, or clear, clients cannot add, update, or remove key/value pairs in the map, limiting its functionality.
Consider adding methods such as:
interface LiveMap extends LiveObject<LiveMapUpdate> {
// Existing methods...
get(key: string): LiveObject | StateValue | undefined;
size(): number;
// Suggested methods
set(key: string, value: LiveObject | StateValue): void;
delete(key: string): boolean;
clear(): void;
}2088-2093:
Missing methods to modify the LiveCounter (e.g., increment, decrement).
The LiveCounter interface provides a value() method but lacks methods to modify the counter. Without methods like increment or decrement, clients cannot update the counter's value, restricting its usability.
Consider adding methods such as:
interface LiveCounter extends LiveObject<LiveCounterUpdate> {
// Existing method...
value(): number;
// Suggested methods
increment(amount: number): void;
decrement(amount: number): void;
}2044-2044: Verify if getRoot() should be asynchronous or synchronous.
The getRoot() method in LiveObjects returns a Promise<LiveMap>, indicating an asynchronous operation. Verify whether this is necessary or if it can be a synchronous method, as this might impact client usage.
Run the following script to check for asynchronous initialization in getRoot() implementations:
✅ Verification successful
The getRoot() method must be asynchronous due to state synchronization requirements
Based on the implementation and tests, the asynchronous nature of getRoot() is necessary because:
- It needs to wait for the initial STATE_SYNC sequence to complete before resolving:
async getRoot(): Promise<LiveMap> {
// SYNC is currently in progress, wait for SYNC sequence to finish
if (this._syncInProgress) {
await this._eventEmitter.once(LiveObjectsEvents.SyncCompleted);
}
// ...
}- Multiple test cases verify this critical behavior:
- Waits for initial STATE_SYNC completion before resolving
- Waits for STATE_SYNC with empty cursor before resolving
- Only resolves immediately when STATE_SYNC sequence is already completed
The asynchronous design is fundamental to ensure data consistency by guaranteeing that the root LiveMap is only returned after the channel's state is properly synchronized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for asynchronous implementations of getRoot() in the codebase.
# Expected: Function definitions of getRoot() that return a Promise
ast-grep --pattern $'getRoot(): Promise<LiveMap> { $$$ }'
Length of output: 58
Script:
#!/bin/bash
# Let's try a broader search for getRoot implementations
ast-grep --pattern 'getRoot() { $$$ }'
# Also search for any getRoot declarations or implementations
rg "getRoot" -A 5 -B 5
# Search specifically in implementation files
fd "live.*\.(ts|js)" --exec rg "getRoot" -A 5 -B 5 {}
Length of output: 32768
This is needed to enable LiveObjects plugin package tests and TypeScript checks.
8471516 to
0b4b1e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ably.d.ts (3)
2038-2045: LGTM with a documentation suggestion.The
LiveObjectsinterface is well-defined. Consider enhancing the JSDoc documentation forgetRoot()to include information about possible error cases and error handling.Add error handling documentation:
/** * Retrieves the root {@link LiveMap} object for state on a channel. * * @returns A promise which, upon success, will be fulfilled with a {@link LiveMap} object. * Upon failure, the promise will be rejected with an {@link ErrorInfo} object * which explains the error, such as when the channel is not attached. */
2097-2110: Consider enhancing documentation for the inc property.The
LiveCounterUpdateinterface is well-defined, but the documentation could be clearer about the semantics of theincproperty.Add clarifying documentation:
/** * The value by which the counter was incremented (positive) or decremented (negative). * For example, inc: 1 means the counter was incremented by 1, inc: -1 means it was decremented by 1. */ inc: number;
2137-2145: Consider using a more specific type for the update field.The
LiveObjectUpdateinterface usesanyfor the update field type. Consider using a more specific type or adding type constraints./** * Represents a generic update object describing the changes that occurred on a Live Object. */ export declare interface LiveObjectUpdate { /** * Holds an update object which describe changes applied to the object. * The structure of this object depends on the specific Live Object type. */ update: Record<string, unknown>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ably.d.ts(2 hunks)
🔇 Additional comments (6)
ably.d.ts (6)
2053-2066: Missing essential Map operations.
The LiveMap interface is missing essential methods for modifying the map (set, delete, clear). This limits the functionality of the synchronized key/value storage.
2071-2078: LGTM!
The LiveMapUpdate interface is well-defined with clear documentation and proper typing for update status indicators.
2080-2085: LGTM!
The StateValue type is well-defined with proper handling of platform-specific binary data types.
2090-2095: Missing essential Counter operations.
The LiveCounter interface is missing essential methods for modifying the counter (increment, decrement). This limits the functionality of the synchronized counter.
2112-2135: LGTM!
The LiveObject base interface is well-designed with proper generic typing and comprehensive subscription management methods.
2147-2155: LGTM!
The SubscribeResponse interface is well-defined with clear documentation and purpose.
mschristensen
left a comment
There was a problem hiding this comment.
Minor suggestions, but looks good :)
Co-authored-by: Mike Christensen <mike.christensen@ably.com>
e971ff9 to
6cbe7ed
Compare
ably.d.tstypes are required to enable LiveObjects package tests in #1921Summary by CodeRabbit
LiveMapandLiveCounter, for synchronized storage and counters.LiveObjectsfor improved data interaction.