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

Restrict presence object type to JSON serializable values #898

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

gwbaik9717
Copy link
Contributor

@gwbaik9717 gwbaik9717 commented Sep 8, 2024

What this PR does / why we need it?

This PR introduces a more restrictive type for the presence object P. Previously, the type <P extends Indexable> allowed any type as a value, since Indexable was defined as Record<string, any>. This broad type allowed values that are not JSON serializable, such as byte arrays, Date, and Long, which could cause issues during serialization.

To address this, we now restrict the type of P to only allow JSON serializable types. This ensures that the presence object contains values that can be safely serialized to JSON.

Any background context you want to provide?

The Json type introduced in this PR is based on a similar approach used by Liveblocks. Here’s the definition of the Json type:

export type Json = JsonScalar | JsonArray | JsonObject;

// eslint-disable-next-line @typescript-eslint/ban-types
type JsonScalar = string | number | boolean | null;
type JsonArray = Array<Json>;
type JsonObject = { [key: string]: Json | undefined };

With the introduction of the Json type, we now enforce that only JSON serializable payloads can be broadcast, addressing issues like #884 . For instance, the broadcast function has been updated as follows:

  /**
   * `broadcast` the payload to the given topic.
   */
  public broadcast(topic: string, payload: Json, error?: ErrorFn) {
  }

What are the relevant tickets?

Fixes #591

Checklist

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

  • New Features

    • Enhanced type safety for payloads in broadcasting methods by changing from any to a specific Json type.
    • Introduced a centralized Json type definition for better consistency across the codebase.
  • Bug Fixes

    • Improved type checks in various methods to prevent runtime errors related to type mismatches.
  • Chores

    • Added a new entry in .eslintignore to exclude a specific TypeScript declaration file from linting checks.

Copy link

coderabbitai bot commented Sep 8, 2024

Walkthrough

The changes in this pull request involve significant modifications to type definitions, particularly enhancing type safety by replacing the any type with a more specific Json type in various methods and interfaces. A centralized Json type definition has been introduced, ensuring consistency across the codebase. Additionally, a new entry has been added to the .eslintignore file to prevent linting errors in a specific TypeScript declaration file. These updates aim to improve maintainability and reduce potential runtime errors.

Changes

Files Change Summary
packages/sdk/src/document/document.ts Changed payload type from any to Json in BroadcastEvent interface and broadcast method. Added centralized Json type definition. Updated Indexable type to use Json.
.eslintignore Added examples/react-tldraw/src/tldraw.d.ts to ignore list.

Assessment against linked issues

Objective Addressed Explanation
Restrict Presence Type to JSON Serializable Types (#591) The changes do not restrict the presence type to only JSON serializable types, as Indexable still allows non-serializable values.
Ensure type safety in payload handling
Centralize JSON type definition across the codebase

Possibly related PRs

  • Introduce broadcast API for event sharing #884: The changes in this PR involve modifications to the BroadcastEvent interface and the broadcast method in the Document class, which are directly related to the type safety improvements and method signature updates in the main PR.

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1eec4d6 and d5af182.

Files selected for processing (2)
  • .eslintignore (1 hunks)
  • packages/sdk/src/document/document.ts (3 hunks)
Files skipped from review due to trivial changes (1)
  • .eslintignore
Additional comments not posted (4)
packages/sdk/src/document/document.ts (4)

388-388: LGTM!

Changing the payload type from any to Json in the BroadcastEvent interface is a good improvement. It aligns with the PR objective to restrict presence type to JSON serializable types, enhancing type safety and preventing potential serialization issues.


427-432: Great addition of the Json type!

Introducing the Json type as a union of JsonPrimitive, JsonArray, and JsonObject provides a structured approach to define JSON serializable types. It enhances type safety by restricting values to only those that are JSON serializable, preventing potential issues with non-serializable values.


439-439: Excellent update to the Indexable type!

Changing the Indexable type definition from Record<string, any> to Record<string, Json> is a great improvement. It aligns with the PR objective to restrict presence type to JSON serializable types and ensures that the presence information of the client is limited to valid JSON values.


2073-2073: Perfect update to the broadcast method!

Changing the payload parameter type from any to Json in the broadcast method is an excellent improvement. It enforces the restriction on the broadcast payload to be JSON serializable, preventing the broadcasting of non-serializable values and enhancing the reliability of the system.


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>.
    • 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 generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @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.

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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4837c7d and 7ad07da.

Files selected for processing (9)
  • packages/sdk/src/client/client.ts (2 hunks)
  • packages/sdk/src/devtools/types.ts (1 hunks)
  • packages/sdk/src/document/change/context.ts (1 hunks)
  • packages/sdk/src/document/document.ts (3 hunks)
  • packages/sdk/src/document/json/tree.ts (2 hunks)
  • packages/sdk/src/util/object.ts (3 hunks)
  • packages/sdk/src/util/validator.ts (1 hunks)
  • packages/sdk/test/integration/client_test.ts (1 hunks)
  • packages/sdk/test/integration/integration_helper.ts (2 hunks)
Files skipped from review due to trivial changes (2)
  • packages/sdk/src/document/change/context.ts
  • packages/sdk/test/integration/client_test.ts
Additional comments not posted (13)
packages/sdk/src/util/validator.ts (1)

20-20: Approved: Enhanced type safety with unknown.

Changing the parameter type from any to unknown in validateSerializable function enhances type safety by enforcing stricter type checks. This is a good practice in TypeScript to ensure that the function handles only explicitly defined types, which can help prevent runtime errors due to incorrect data types.

packages/sdk/src/util/object.ts (2)

17-17: Approved: Import of Indexable type.

The addition of the Indexable type import from @yorkie-js-sdk/src/document/document enhances type specificity in the stringifyObjectValues and parseObjectValues functions. This change aligns with the PR's objective to improve type safety.


45-45: Approved: Enhanced type safety in utility functions.

Modifying the type parameter from <A extends object> to <A extends Indexable> in both stringifyObjectValues and parseObjectValues functions improves type safety and ensures that these functions can handle objects that are specifically designed to be indexable. This adjustment reduces the risk of runtime errors associated with type mismatches and enhances the robustness of the code.

Also applies to: 58-58

packages/sdk/test/integration/integration_helper.ts (1)

Line range hint 16-36: Approved: Enhanced type safety and flexibility in withTwoClientsAndDocuments.

The modification to include an additional generic type parameter P that extends Indexable in the withTwoClientsAndDocuments function enhances type safety and flexibility. This change allows the documents handled within the function to utilize a more specific type, Document<T, P>, ensuring that they can accommodate additional properties defined by P. This adjustment enhances the function's capability to work with a broader range of document types while maintaining type integrity.

packages/sdk/src/devtools/types.ts (1)

20-20: Approved import change for Json.

The centralization of the Json type is a good practice for consistency and maintainability. Ensure that the import path @yorkie-js-sdk/src/document/document is correct and does not introduce any circular dependencies.

Verification successful

Verified import path for Json.

The import path @yorkie-js-sdk/src/document/document is correct, and the Json type is defined in the expected location. No issues with circular dependencies were found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the import path for Json type.

# Test: Search for the file to confirm the path. Expect: Path exists.
fd --type file 'document/document.ts'

Length of output: 37


Script:

#!/bin/bash
# Description: Search for the definition of the Json type in the codebase.

# Test: Search for the Json type definition. Expect: Find the correct file and path.
ast-grep --lang typescript --pattern 'type Json = $_'

Length of output: 153

packages/sdk/src/document/json/tree.ts (2)

89-92: Approved changes to handle attributes immutably.

The introduction of stringifiedAttributes in the buildDescendants function is a good practice. It enhances code clarity and robustness by ensuring that the original attributes object is not altered. This change helps prevent bugs and side effects associated with mutable data structures.


128-131: Approved consistent handling of attributes.

The use of stringifiedAttributes in the createCRDTTreeNode function mirrors the changes made in buildDescendants. This consistency in handling attributes immutably across different functions is commendable and enhances maintainability.

packages/sdk/src/client/client.ts (2)

611-611: Approved type change for payload in broadcast method.

Changing the payload parameter's type from any to Json enhances type safety and aligns with the PR's objectives. Ensure that the method correctly handles the new type and that there are no issues with serialization.

Verification successful

Verified: The broadcast method correctly handles the Json type for the payload parameter.

The method serializes the payload using JSON.stringify, which is appropriate for JSON data. Integration tests confirm that the method behaves correctly with both serializable and unserializable payloads, ensuring robust handling of JSON serialization. No issues found with the implementation.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the handling of Json type in the broadcast method.

# Test: Search for the method implementation. Expect: Correct handling of Json type.
rg --type python -A 5 $'broadcast'

Length of output: 64


Script:

#!/bin/bash
# Description: Verify the handling of Json type in the broadcast method in TypeScript files.

# Test: Search for the method implementation in TypeScript files. Expect: Correct handling of Json type.
rg --type ts -A 5 $'broadcast'

Length of output: 19965


46-46: Approved import change for Json.

The centralization of the Json type is a good practice for consistency and maintainability. Ensure that the import path @yorkie-js-sdk/src/document/document is correct and does not introduce any circular dependencies.

Verification successful

Verified Import Path for Json Type

The import path @yorkie-js-sdk/src/document/document is correctly configured using a path alias in the tsconfig.json file. The Json type is defined in the expected location, confirming the validity of the import statement.

  • The path alias @yorkie-js-sdk/src/* is correctly mapped to src/* in packages/sdk/tsconfig.json.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the import path for Json type.

# Test: Search for the file to confirm the path. Expect: Path exists.
fd --type file 'document/document.ts'

Length of output: 37


Script:

#!/bin/bash
# Search for the definition of the Json type in the repository.
ast-grep --lang typescript --pattern 'type Json = $_'

Length of output: 153


Script:

#!/bin/bash
# Search for path alias configurations in the project.
rg '"paths"' -A 5 --glob '**/tsconfig*.json'

Length of output: 2595

packages/sdk/src/document/document.ts (4)

388-388: Updated BroadcastEvent interface to use Json type for payload.

The change from any to Json for the payload type in the BroadcastEvent interface aligns with the PR's goal to ensure type safety by restricting types to JSON serializable values. This modification is crucial for preventing runtime errors due to type mismatches and ensures that the data structure is predictable and consistent across the system.


423-429: Definition of Json, JsonScalar, JsonArray, and JsonObject types.

The introduction of these types is a significant enhancement in type safety for the SDK. Json as a union of JsonScalar, JsonArray, and JsonObject provides a clear and enforceable structure for JSON serializable data. This change supports the SDK's robustness by ensuring that only compatible data types are used throughout the system.


434-434: Updated Indexable type to use Json.

Changing the Indexable type from Record<string, any> to Record<string, Json> is a critical update that aligns with the PR's objectives to enforce JSON serializability of the presence types. This change further tightens the type constraints and enhances the predictability and safety of the data handling within the SDK.


2068-2068: Updated broadcast method to accept Json type for payload.

The update of the broadcast method to accept a payload of type Json instead of any is a direct implementation of the PR's goal to enhance type safety. This change ensures that the method's payload is restricted to JSON serializable types, which is essential for consistent data serialization and transmission.

@gwbaik9717
Copy link
Contributor Author

gwbaik9717 commented Sep 8, 2024

There's an issue with Presence type in one of examples, react-tldraw. I'll reopen the PR when ready.

@gwbaik9717 gwbaik9717 closed this Sep 8, 2024
@gwbaik9717 gwbaik9717 reopened this Sep 9, 2024
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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7ad07da and b8b571e.

Files selected for processing (5)
  • .eslintignore (1 hunks)
  • examples/react-tldraw/src/tldraw.d.ts (1 hunks)
  • examples/vanilla-quill/src/main.ts (1 hunks)
  • examples/vanilla-quill/src/type.ts (1 hunks)
  • packages/sdk/src/document/json/tree.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • .eslintignore
Files skipped from review as they are similar to previous changes (1)
  • packages/sdk/src/document/json/tree.ts
Additional comments not posted (3)
examples/react-tldraw/src/tldraw.d.ts (1)

1-6: LGTM!

The changes introduce a new type definition file for the @tldraw/tldraw module. The TDUser interface is being extended to include the Indexable type, which is imported from @yorkie-js-sdk/src/document/document. This change allows the TDUser interface to have additional properties of type Json, as defined by the Indexable type.

examples/vanilla-quill/src/type.ts (1)

10-10: Verify the impact of making the selection property optional.

The selection property in the YorkiePresence type has been changed from a mandatory field to an optional field. This change allows instances of YorkiePresence to omit the selection property entirely.

Ensure that the code that interacts with YorkiePresence is updated to handle the case where selection is absent. Verify that this change does not introduce any unexpected behavior or break the functionality related to presence state management.

examples/vanilla-quill/src/main.ts (1)

32-32: Verify the expected format of the embed value and its impact on the functionality.

The toDeltaOperation function has been modified to directly return the embed value without parsing it. This change suggests that the embed is now treated as a raw value rather than a JSON string.

Ensure that the code that relies on the embed data is updated to handle the new format. Verify that this change does not introduce any unexpected behavior or break the functionality related to handling embedded data in the Quill editor.

Consider adding documentation or comments to clarify the expected format of the embed value and how it should be used.

@gwbaik9717
Copy link
Contributor Author

gwbaik9717 commented Sep 9, 2024

@chacha912

Since the Indexable type has been changed to Record<string, Json>, type issues have surfaced in some examples, in react-tldraw and vanilla-quill.

1. react-tldraw

In react-tldraw, we use YorkiePresenceType for Presence, which is defined as follows:

export type YorkiePresenceType = {
  tdUser: TDUser;
};

However, TDUser from @tldraw/tldraw is not compatible with our Json type. (still figuring out the reason, possibly one of its fields includes undefined...) Since TDUser is an imported type and not something we can directly control, I've decided to force the compatibility with our Indexable type by declaring tldraw.d.ts.

2. vanilla-quill

Simillarly In vanilla-quil, the Presence type was previously defined as:

export type YorkiePresence = {
  // before
  selection: TextPosStructRange | undefined;

  // after
  selection?: TextPosStructRange
};

However, undefined is not serializable and thus is not compatible with the Json type. To resolve this, I've changed selection to be optional (?). While this fixes the TypeScript type issues, it could potentially introduce issues in the actual implementation since the undefined value may still be present in some parts of the code.

Additionally, I encountered a type issue in src/main.ts within the toDeltaOperation function. Previously, Json.parse was being called on a Json type, which caused errors. I removed this part, and while the TypeScript errors have been resolved, we need confirmation that this change is correct and won't lead to issues.

Here’s the updated toDeltaOperation function:

function toDeltaOperation<T extends TextValueType>(
   const { embed, ...restAttributes } = textValue.attributes ?? {};
    if (embed) {
      return { insert: embed, attributes: restAttributes };
    }
}

Above changes are all included in the latest commit; please check if it okay :)

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@hackerwins hackerwins changed the title Introduce Json Type to Restrict Presence Type to JSON Serializable Types Restrict presence object type to JSON serializable values Sep 12, 2024
@hackerwins hackerwins merged commit 4d5416f into main Sep 12, 2024
2 checks passed
@hackerwins hackerwins deleted the serializable-type branch September 12, 2024 05:58
JOOHOJANG pushed a commit that referenced this pull request Oct 22, 2024
To prevent serialization issues, this commit narrows the type of the `presence`
object `P`. Previously, `<P extends Indexable>` allowed any value type, 
including non-JSON serializable ones like byte arrays, Date, and Long.

We now introduce a `Json` type, inspired by Liveblocks, to ensure only JSON
serializable types are allowed in the presence object. This change affects
functions like `broadcast`, ensuring safe serialization and addressing 
issues such as #884.

The new `Json` type is defined as follows:

```
export type Json = JsonPrimitive | JsonArray | JsonObject;
type JsonPrimitive = string | number | boolean | null;
type JsonArray = Array<Json>;
type JsonObject = { [key: string]: Json | undefined };
```

This type restriction enhances type safety and prevents potential runtime 
errors during JSON serialization of presence data.

---------

Co-authored-by: Youngteac Hong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Restrict Presence Type to JSON Serializable Types
2 participants