Skip to content
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

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Nov 28, 2024

ably.d.ts types are required to enable LiveObjects package tests in #1921

Summary by CodeRabbit

  • New Features
    • Introduced real-time data structures, including LiveMap and LiveCounter, for synchronized storage and counters.
    • Added methods for managing updates and subscriptions to live objects.
    • Enhanced TypeScript definitions with new interfaces and types for better usability and functionality.
    • Added a method to retrieve the root of LiveObjects for improved data interaction.

Copy link

coderabbitai bot commented Nov 28, 2024

Caution

Review failed

The pull request is closed.

Walkthrough

The 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 LiveObjects interface with a getRoot() method, the LiveMap interface for synchronized key/value storage, and the LiveCounter interface for synchronized counters. Additional interfaces such as LiveObject, StateValue, and SubscribeResponse are defined to manage updates and subscriptions, facilitating better synchronization and management of live data across clients.

Changes

Files Change Summary
ably.d.ts - Added LiveObjects interface with getRoot() method.
- Introduced LiveMap interface with get(key: string) and size() methods.
- Added LiveMapUpdate interface with update property.
- Introduced StateValue, LiveCounter, and LiveCounterUpdate interfaces.
- Added value() method in LiveCounter and update property in LiveCounterUpdate.
- Defined LiveObject interface with methods for subscribing and unsubscribing to updates.
- Added LiveObjectUpdate and SubscribeResponse interfaces with relevant properties/methods.

Possibly related PRs

Suggested reviewers

  • mschristensen
  • owenpearson

🐰 In the meadow, where data flows,
New structures sprout, as the knowledge grows.
LiveObjects dance, with counters in tow,
Synchronized magic, watch them glow!
With maps and updates, they play and weave,
In this realm of code, we all believe! 🌼

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 0b4b1e4 and 6cbe7ed.


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/1928/bundle-report November 28, 2024 04:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/features November 28, 2024 04:39 Inactive
Copy link

@coderabbitai coderabbitai bot left a 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.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 3adafa5 and bf0ffeb.

📒 Files selected for processing (1)
  • ably.d.ts (1 hunks)

ably.d.ts Show resolved Hide resolved
ably.d.ts Show resolved Hide resolved
@VeskeR VeskeR force-pushed the liveobjects/read-api-docs branch from bf0ffeb to 8471516 Compare November 28, 2024 05:13
@github-actions github-actions bot temporarily deployed to staging/pull/1928/bundle-report November 28, 2024 05:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/features November 28, 2024 05:14 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/typedoc November 28, 2024 05:14 Inactive
Copy link

@coderabbitai coderabbitai bot left a 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 to LiveMap.

The LiveMap interface currently lacks methods to iterate over its entries, keys, or values. Adding methods like entries(), keys(), and values() 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 the LiveCounterUpdate interface.

The LiveCounterUpdate interface specifies an update property with an inc value. Consider generalizing this to a delta 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 in LiveObjectUpdateCallback type definition.

There's a minor typographical error in the comment for LiveObjectUpdateCallback. The sentence "A callback used in LiveObject 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 in getRoot() 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 an ErrorInfo object upon failure.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between bf0ffeb and 8471516.

📒 Files selected for processing (1)
  • ably.d.ts (2 hunks)
🔇 Additional comments (3)
ably.d.ts (3)

2053-2066: ⚠️ Potential issue

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: ⚠️ Potential issue

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:

  1. 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);
  }
  // ...
}
  1. 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

@VeskeR VeskeR requested a review from mschristensen November 28, 2024 06:33
This is needed to enable LiveObjects plugin package tests and TypeScript
checks.
@VeskeR VeskeR force-pushed the liveobjects/read-api-docs branch from 8471516 to 0b4b1e4 Compare November 28, 2024 06:43
@github-actions github-actions bot temporarily deployed to staging/pull/1928/bundle-report November 28, 2024 06:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/features November 28, 2024 06:44 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/typedoc November 28, 2024 06:44 Inactive
Copy link

@coderabbitai coderabbitai bot left a 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 for getRoot() 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 the inc 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 uses any 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8471516 and 0b4b1e4.

📒 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.

Copy link
Contributor

@mschristensen mschristensen left a 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 :)

ably.d.ts Outdated Show resolved Hide resolved
ably.d.ts Outdated Show resolved Hide resolved
ably.d.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to staging/pull/1928/features November 29, 2024 06:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/bundle-report November 29, 2024 06:20 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1928/typedoc November 29, 2024 06:20 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants