Restrict presence object type to JSON serializable values #898
Conversation
WalkthroughThe changes in this pull request involve significant modifications to type definitions, particularly enhancing type safety by replacing the Changes
Assessment against linked issues
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Additional comments not posted (4)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 withunknown.Changing the parameter type from
anytounknowninvalidateSerializablefunction 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 ofIndexabletype.The addition of the
Indexabletype import from@yorkie-js-sdk/src/document/documentenhances type specificity in thestringifyObjectValuesandparseObjectValuesfunctions. 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 bothstringifyObjectValuesandparseObjectValuesfunctions 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 inwithTwoClientsAndDocuments.The modification to include an additional generic type parameter
Pthat extendsIndexablein thewithTwoClientsAndDocumentsfunction 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 byP. 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 forJson.The centralization of the
Jsontype is a good practice for consistency and maintainability. Ensure that the import path@yorkie-js-sdk/src/document/documentis correct and does not introduce any circular dependencies.Verification successful
Verified import path for
Json.The import path
@yorkie-js-sdk/src/document/documentis correct, and theJsontype 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
stringifiedAttributesin thebuildDescendantsfunction 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
stringifiedAttributesin thecreateCRDTTreeNodefunction mirrors the changes made inbuildDescendants. 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 forpayloadinbroadcastmethod.Changing the
payloadparameter's type fromanytoJsonenhances 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
broadcastmethod correctly handles theJsontype for thepayloadparameter.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 forJson.The centralization of the
Jsontype is a good practice for consistency and maintainability. Ensure that the import path@yorkie-js-sdk/src/document/documentis correct and does not introduce any circular dependencies.Verification successful
Verified Import Path for
JsonTypeThe import path
@yorkie-js-sdk/src/document/documentis correctly configured using a path alias in thetsconfig.jsonfile. TheJsontype is defined in the expected location, confirming the validity of the import statement.
- The path alias
@yorkie-js-sdk/src/*is correctly mapped tosrc/*inpackages/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: UpdatedBroadcastEventinterface to useJsontype for payload.The change from
anytoJsonfor thepayloadtype in theBroadcastEventinterface 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 ofJson,JsonScalar,JsonArray, andJsonObjecttypes.The introduction of these types is a significant enhancement in type safety for the SDK.
Jsonas a union ofJsonScalar,JsonArray, andJsonObjectprovides 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: UpdatedIndexabletype to useJson.Changing the
Indexabletype fromRecord<string, any>toRecord<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: Updatedbroadcastmethod to acceptJsontype for payload.The update of the
broadcastmethod to accept apayloadof typeJsoninstead ofanyis 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.
|
There's an issue with |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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/tldrawmodule. TheTDUserinterface is being extended to include theIndexabletype, which is imported from@yorkie-js-sdk/src/document/document. This change allows theTDUserinterface to have additional properties of typeJson, as defined by theIndexabletype.examples/vanilla-quill/src/type.ts (1)
10-10: Verify the impact of making theselectionproperty optional.The
selectionproperty in theYorkiePresencetype has been changed from a mandatory field to an optional field. This change allows instances ofYorkiePresenceto omit theselectionproperty entirely.Ensure that the code that interacts with
YorkiePresenceis updated to handle the case whereselectionis 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 theembedvalue and its impact on the functionality.The
toDeltaOperationfunction has been modified to directly return theembedvalue without parsing it. This change suggests that theembedis now treated as a raw value rather than a JSON string.Ensure that the code that relies on the
embeddata 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
embedvalue and how it should be used.
|
Since the 1. react-tldraw In react-tldraw, we use export type YorkiePresenceType = {
tdUser: TDUser;
};However, 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, Additionally, I encountered a type issue in Here’s the updated Above changes are all included in the latest commit; please check if it okay :) |
hackerwins
force-pushed
the
serializable-type
branch
from
1eec4d6
to
d5af182
Compare
hackerwins
changed the title
Json Type to Restrict Presence Type to JSON Serializable Types
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]>
What this PR does / why we need it?
This PR introduces a more restrictive type for the
presenceobjectP. Previously, the type<P extends Indexable>allowedanytype as a value, sinceIndexablewas defined asRecord<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
Pto 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
Jsontype introduced in this PR is based on a similar approach used by Liveblocks. Here’s the definition of theJsontype:With the introduction of the
Jsontype, we now enforce that only JSON serializable payloads can be broadcast, addressing issues like #884 . For instance, thebroadcastfunction has been updated as follows:What are the relevant tickets?
Fixes #591
Checklist
Summary by CodeRabbit
New Features
anyto a specificJsontype.Jsontype definition for better consistency across the codebase.Bug Fixes
Chores
.eslintignoreto exclude a specific TypeScript declaration file from linting checks.