-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LiveObjects read-only public API docs (ably.d.ts
types)
#1928
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
ably.d.ts (4)
2067-2079
: Consider adding iteration methods toLiveMap
.The
LiveMap
interface 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 theLiveCounterUpdate
interface.The
LiveCounterUpdate
interface specifies anupdate
property with aninc
value. Consider generalizing this to adelta
property 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 inLiveObjectUpdateCallback
type definition.There's a minor typographical error in the comment for
LiveObjectUpdateCallback
. The sentence "A callback used inLiveObject
to 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
LiveMap
object upon success or rejects with anErrorInfo
object 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ably.d.ts (3)
2038-2045
: LGTM with a documentation suggestion.The
LiveObjects
interface 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
LiveCounterUpdate
interface is well-defined, but the documentation could be clearer about the semantics of theinc
property.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
LiveObjectUpdate
interface usesany
for 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor suggestions, but looks good :)
Co-authored-by: Mike Christensen <[email protected]>
e971ff9
to
6cbe7ed
Compare
ably.d.ts
types are required to enable LiveObjects package tests in #1921Summary by CodeRabbit
LiveMap
andLiveCounter
, for synchronized storage and counters.LiveObjects
for improved data interaction.