[DTP-952, DTP-953] Add base implementation for abstract LiveObject, concrete LiveMap/LiveCounter classes

[VeskeR]

This PR is based on #1880, please review it first.

Resolves DTP-952, DTP-953

Summary by CodeRabbit

Release Notes

• New Features

  • Introduced LiveCounter and LiveMap classes for managing live data with enhanced type safety.
  • Added LiveObjectsPool to manage collections of live objects efficiently.
  • Implemented a new asynchronous method getRoot() in LiveObjects to retrieve the root live object.

• Tests

  • Added new test cases for the LiveObjects plugin to validate the functionality of the getRoot() method.

• Chores

  • Updated the PrivateApiRecorder to track new private API calls related to LiveObject.

[coderabbitai]

Warning

Rate limit exceeded

@VeskeR has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 5 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

Files that changed from the base of the PR and between fe80195 and d7211a4.

Walkthrough

The changes introduce several new TypeScript files and classes related to managing live objects in a system. Key additions include the LiveCounter and LiveMap classes, which extend the LiveObject class, alongside the LiveObjectsPool for managing collections of live objects. The LiveObjects class is updated to include a method for retrieving the root live object. Test cases are added to validate the functionality of the getRoot() method in the LiveObjects plugin, ensuring it returns the expected LiveMap instance.

Changes

File Path	Change Summary
src/plugins/liveobjects/livecounter.ts	Added LiveCounter class and LiveCounterData interface; implemented _getZeroValueData() method.
src/plugins/liveobjects/livemap.ts	Introduced LiveMap class, LiveMapData interface, and related types/interfaces for managing state data.
src/plugins/liveobjects/liveobject.ts	Added abstract LiveObject class with methods for managing live object instances and zero-value data generation.
src/plugins/liveobjects/liveobjects.ts	Modified LiveObjects class to include a private LiveObjectsPool member and a new getRoot() method.
src/plugins/liveobjects/liveobjectspool.ts	Introduced LiveObjectsPool class for managing LiveObject instances, including methods for object retrieval.
test/common/modules/private_api_recorder.js	Updated privateAPIIdentifiers to include call.LiveObject.getObjectId.
test/realtime/live_objects.test.js	Added tests for LiveObjects plugin to validate getRoot() method functionality.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant LiveObjects
    participant LiveMap
    participant LiveObjectsPool

    User->>LiveObjects: getRoot()
    LiveObjects->>LiveObjectsPool: get(ROOT_OBJECT_ID)
    LiveObjectsPool-->>LiveObjects: LiveMap instance
    LiveObjects-->>User: LiveMap instance

Loading

🐰 "In the meadow where we play,
New counters hop and maps display.
Live objects dance, a joyful sight,
With roots and pools, they take flight!
Let's celebrate this code so bright!" 🐇✨

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 4

🧹 Outside diff range and nitpick comments (12)

src/plugins/liveobjects/livecounter.ts (1)

1-11: Summary: Good foundation for LiveCounter, consider expanding functionality.

Overall, this file provides a solid foundation for the LiveCounter implementation. It's consistent with the PR objectives and introduces the necessary base structures. The code is clean, well-organized, and type-safe.

To further improve this implementation, consider:

1. Adding public methods for common counter operations.
2. Including documentation comments for the class and its methods.
3. Potentially adding error handling for edge cases (e.g., overflow).

These enhancements would make the LiveCounter class more robust and user-friendly while maintaining its current simplicity.

src/plugins/liveobjects/liveobjectspool.ts (2)

15-17: LGTM: get method is correctly implemented. Consider adding a set method.

The get method is implemented correctly, using the Map's get method to retrieve LiveObject instances by their ObjectId.

Consider adding a set method to allow adding new objects to the pool after initialization. This would make the class more flexible for future use cases. Here's a suggested implementation:

set(objectId: ObjectId, liveObject: LiveObject): void {
  this._pool.set(objectId, liveObject);
}

---

19-24: LGTM: _getInitialPool method is well-implemented. Consider caching the root object.

The _getInitialPool method correctly initializes the pool with a root LiveMap object. The use of the ROOT_OBJECT_ID constant is good practice.

Consider caching the root object to prevent creating multiple root objects if this method is called more than once. Here's a suggested implementation:

private _rootObject: LiveMap | null = null;

private _getInitialPool(): Map<ObjectId, LiveObject> {
  const pool = new Map<ObjectId, LiveObject>();
  if (!this._rootObject) {
    this._rootObject = new LiveMap(this._liveObjects, null, ROOT_OBJECT_ID);
  }
  pool.set(this._rootObject.getObjectId(), this._rootObject);
  return pool;
}

This ensures that only one root object is created, even if _getInitialPool is called multiple times.

test/realtime/live_objects.test.js (1)

61-76: LGTM! Consider adding a descriptive comment.

The test case effectively verifies that getRoot() returns a LiveMap instance. It follows good practices such as proper connection management and the AAA pattern.

Consider adding a brief comment explaining the purpose of this test case for improved readability:

/** @nospec */
it('getRoot() returns LiveMap instance', async function () {
  // This test verifies that the root object returned by getRoot() is an instance of LiveMap
  const helper = this.test.helper;
  // ... rest of the test case
});

test/common/modules/private_api_recorder.js (1)

47-47: LGTM! Consider adding a comment for the new LiveObject API.

The addition of 'call.LiveObject.getObjectId' to the privateAPIIdentifiers array is correct and aligns with the PR objectives. It follows the existing naming convention and maintains the alphabetical order.

Consider adding a brief comment above this new entry to explain its purpose or link it to the relevant issue (DTP-952 or DTP-953). This would help maintain consistency with any existing comments and improve code documentation.

src/plugins/liveobjects/livemap.ts (6)

5-10: Add 'readonly' modifier to 'ObjectIdStateData' properties for immutability

To ensure that instances of ObjectIdStateData remain immutable once created, consider adding the readonly modifier to the objectId property. This can help prevent accidental mutations, enhancing code safety and predictability.

Apply this diff to add the readonly modifier:

 export interface ObjectIdStateData {
   /**
    * A reference to another state object, used to support composable state objects.
    */
-  objectId: string;
+  readonly objectId: string;
 }

---

12-17: Add 'readonly' modifier to 'ValueStateData' properties for immutability

Similarly, to enforce immutability in the ValueStateData interface, you can add the readonly modifier to the value property.

Apply this diff:

 export interface ValueStateData {
   /**
    * A concrete leaf value in the state object graph.
    */
-  value: StateValue;
+  readonly value: StateValue;
 }

---

21-24: Consider renaming the 'data' property in 'MapEntry' for clarity

The MapEntry interface uses a property named data, which might be too generic and could cause confusion with other data properties in the codebase. Renaming it to something more descriptive like stateData can improve readability and maintainability.

Apply this diff to rename the property:

 export interface MapEntry {
   // TODO: add tombstone, timeserial
-  data: StateData;
+  stateData: StateData;
 }

Remember to update all references to data within the MapEntry context accordingly.

---

26-28: Consider renaming the 'data' property in 'LiveMapData' for clarity

In the LiveMapData interface, the property data holds a Map of entries. Renaming this property to entries or mapEntries can make its purpose clearer and differentiate it from other data properties.

Apply this diff:

 export interface LiveMapData {
-  data: Map<string, MapEntry>;
+  entries: Map<string, MapEntry>;
 }

Ensure that you update all references to this property in the codebase.

---

22-22: Address the TODO comment regarding 'tombstone' and 'timeserial'

There's a TODO in the MapEntry interface to add tombstone and timeserial properties. Implementing these properties might be crucial for features like conflict resolution and state synchronization.

Would you like assistance in defining and implementing the tombstone and timeserial properties? I can help draft the necessary code or open a GitHub issue to track this task.

---

30-34: Consider making 'LiveMap' generic over key and value types

Currently, LiveMapData uses a Map<string, MapEntry>, which restricts keys to strings. If there is a possibility of using different key types or you want more flexibility, consider parameterizing LiveMap over key and value types.

Here's how you might adjust the code:

-export interface LiveMapData {
-  entries: Map<string, MapEntry>;
+export interface LiveMapData<K, V> {
+  entries: Map<K, V>;
 }

-export class LiveMap extends LiveObject<LiveMapData> {
+export class LiveMap<K, V> extends LiveObject<LiveMapData<K, V>> {
   protected _getZeroValueData(): LiveMapData<K, V> {
     return { entries: new Map<K, V>() };
   }
 }

This change increases the flexibility and reusability of the LiveMap class but also adds complexity. Evaluate whether this aligns with your design goals.

src/plugins/liveobjects/liveobject.ts (1)

28-28: Address the TODO: Implement object ID generation based on type and initial value.

The TODO comment indicates that the object ID should be generated based on the live object type and its initial value. Implementing this will provide more meaningful and consistent IDs, which could be beneficial for debugging and tracking.

Would you like assistance in implementing this functionality? I can help draft the code or open a GitHub issue to track this task.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between d9d6408 and fe80195.

📒 Files selected for processing (7)

• src/plugins/liveobjects/livecounter.ts (1 hunks)
• src/plugins/liveobjects/livemap.ts (1 hunks)
• src/plugins/liveobjects/liveobject.ts (1 hunks)
• src/plugins/liveobjects/liveobjects.ts (1 hunks)
• src/plugins/liveobjects/liveobjectspool.ts (1 hunks)
• test/common/modules/private_api_recorder.js (1 hunks)
• test/realtime/live_objects.test.js (1 hunks)

🔇 Additional comments (13)

src/plugins/liveobjects/livecounter.ts (3)

1-1: LGTM: Import statement is correct.

The import statement correctly imports the LiveObject class, which is used as the base class for LiveCounter.

---

3-5: LGTM: LiveCounterData interface is well-defined.

The LiveCounterData interface is simple, clear, and appropriate for its purpose. It provides a type-safe structure for the counter data.

---

7-11: LGTM: LiveCounter class is correctly implemented.

The LiveCounter class correctly extends LiveObject<LiveCounterData> and provides a protected method _getZeroValueData() to initialize the counter at zero. The implementation is simple and appropriate for a base counter class.

src/plugins/liveobjects/liveobjects.ts (3)

3-4: LGTM: New imports added correctly.

The new imports for LiveMap, LiveObjectsPool, and ROOT_OBJECT_ID are correctly added and necessary for the new functionality being introduced in this file.

---

9-9: LGTM: New private member variable added correctly.

The new private member variable _liveObjectsPool of type LiveObjectsPool is correctly declared. It follows the appropriate naming convention for private members and includes explicit type declaration, which is good TypeScript practice.

---

17-20: ⚠️ Potential issue

Implementation incomplete and potential type safety issue.

The getRoot method has been added, but there are a few concerns:

1. The TODO comment indicates that the implementation is incomplete. The method should wait for the SYNC sequence to finish before returning the root object to avoid potential race conditions.

2. The use of as LiveMap for type assertion might be unsafe if the root object is not guaranteed to be a LiveMap. Consider using a type guard or a more robust type checking mechanism.

To address these issues:

1. Implement the waiting mechanism for the SYNC sequence.
2. Replace the type assertion with a safer type checking approach.

Here's a potential implementation (note that this is a suggestion and may need to be adapted based on your specific synchronization mechanism):

async getRoot(): Promise<LiveMap> {
  await this.waitForSync(); // Implement this method to wait for synchronization
  const rootObject = this._liveObjectsPool.get(ROOT_OBJECT_ID);
  if (rootObject instanceof LiveMap) {
    return rootObject;
  }
  throw new Error('Root object is not a LiveMap');
}

To ensure that ROOT_OBJECT_ID is correctly defined and used consistently, let's check its definition:

#!/bin/bash
# Description: Verify the definition and usage of ROOT_OBJECT_ID

# Test: Search for the ROOT_OBJECT_ID definition
rg --type typescript 'ROOT_OBJECT_ID'

src/plugins/liveobjects/liveobjectspool.ts (3)

1-6: LGTM: Imports and type definitions are well-structured.

The imports are appropriate for the class functionality. The ObjectId type alias and ROOT_OBJECT_ID constant improve code readability and maintainability.

---

8-13: LGTM: Class declaration and constructor are well-implemented.

The LiveObjectsPool class is structured correctly, following TypeScript best practices. The use of a Map for the pool is appropriate for efficient storage and retrieval of LiveObject instances by their ObjectId.

---

1-25: Overall, the LiveObjectsPool class is well-implemented with room for minor enhancements.

The class provides a solid foundation for managing LiveObject instances. Consider implementing the suggested improvements:

1. Add a set method for adding new objects to the pool.
2. Optimize the _getInitialPool method to cache the root object.

These enhancements will improve the flexibility and efficiency of the LiveObjectsPool class.

test/realtime/live_objects.test.js (2)

59-96: Overall, good addition to the test suite.

The new test cases for the LiveObjects instance provide valuable coverage for the getRoot() method. They verify both the type of the returned object (LiveMap) and its ID ("root"), which are crucial aspects of the functionality.

The tests are well-structured, consistent with existing patterns in the file, and properly handle asynchronous operations. They also ensure proper connection management through the use of the monitorConnectionThenCloseAndFinish helper function.

A few minor suggestions for improvement:

1. Consider adding brief comments to explain the purpose of each test case.
2. Clarify the intention behind using recordPrivateApi for the getObjectId() method.

These additions strengthen the test suite and increase confidence in the LiveObjects plugin functionality.

---

79-95: LGTM! Consider clarifying the use of private API.

The test case effectively verifies that the root object returned by getRoot() has the ID "root". It follows good practices such as proper connection management and the AAA pattern.

The use of helper.recordPrivateApi('call.LiveObject.getObjectId') suggests that getObjectId() is a private method. Could you clarify if this is intentional? If getObjectId() is meant to be a public API, consider removing the recordPrivateApi call. If it's intended to remain private, we might want to reconsider testing it directly in a public-facing test.

To verify the usage of getObjectId() method, let's run the following script:

This script will help us understand how getObjectId() is defined and used throughout the codebase, which can inform our decision on whether it should be treated as a private or public method.

src/plugins/liveobjects/livemap.ts (1)

3-3: Extend 'StateValue' to include 'null' and 'undefined' if needed

Currently, the StateValue type includes string, number, boolean, and Uint8Array. If there is a need to represent empty or optional values, you might consider including null and undefined.

Please verify if null or undefined should be acceptable StateValues. If so, update the type accordingly:

 export type StateValue = string | number | boolean | Uint8Array;
+// Include null and undefined if necessary
+// export type StateValue = string | number | boolean | Uint8Array | null | undefined;

src/plugins/liveobjects/liveobject.ts (1)

3-5: ⚠️ Potential issue

Avoid using the any type in LiveObjectData.

Using any defeats the purpose of TypeScript's type safety. Consider defining a more specific type or making LiveObjectData a generic interface to enhance type safety and catch potential errors at compile time.

Apply this diff to improve type safety:

-interface LiveObjectData {
-  data: any;
-}
+interface LiveObjectData<TData = unknown> {
+  data: TData;
+}

Then adjust the class declaration accordingly:

-export abstract class LiveObject<T extends LiveObjectData = LiveObjectData> {
+export abstract class LiveObject<T extends LiveObjectData<any> = LiveObjectData> {

⛔ Skipped due to learnings

Learnt from: VeskeR
PR: ably/ably-js#1880
File: liveobjects.d.ts:26-26
Timestamp: 2024-10-01T05:35:02.125Z
Learning: For plugins like `LiveObjects`, the `any` type is intentionally used to maintain flexibility.

[mschristensen]

Nice :)