Implement InitialRoot option for Document attachment
#913
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Possibly related issues
Poem
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: 3
🧹 Outside diff range and nitpick comments (1)
packages/sdk/test/integration/document_test.ts (1)
1254-1254: Simplify object destructuring by removing unnecessary renamingIn the destructuring assignment, the renaming
name: nameis unnecessary since the property name and variable name are identical. You can simplify it by just usingname.Apply this diff to simplify the code:
-for (const { name: name, input, expectedJSON } of testCases) { +for (const { name, input, expectedJSON } of testCases) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/src/client/client.ts (2 hunks)
- packages/sdk/test/integration/document_test.ts (4 hunks)
🧰 Additional context used
🪛 Biome
packages/sdk/test/integration/document_test.ts
[error] 1258-1258: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
[error] 1278-1279: Don't use 'Object' as a type.
Prefer explicitly define the object shape. This type means "any non-nullable value", which is slightly better than 'unknown', but it's still a broad type.
(lint/complexity/noBannedTypes)
🔇 Additional comments (2)
packages/sdk/src/client/client.ts (2)
300-300: LGTM: NewinitialRootoption added toattachmethod.The
initialRootoption allows setting initial values for the document's root object when attaching it to the client. This addition enhances the flexibility of document initialization.
Line range hint
300-381: Summary:initialRootoption enhances document initialization flexibility.The addition of the
initialRootoption in theattachmethod provides a way to set initial values for the document's root object when attaching it to the client. This change enhances the SDK's flexibility in document initialization.The implementation is correct, but could benefit from some type safety improvements as suggested in the previous comment. The change is well-contained and doesn't appear to have any negative impact on the rest of the
Clientclass or file.Overall, this is a valuable addition to the SDK's functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/sdk/test/integration/document_test.ts (4)
1116-1121: Consider adding error handling test cases.The test verifies successful initialization but doesn't cover error scenarios. Consider adding test cases for:
- Invalid initial root structure
- Malformed counter types
- Invalid data types
Would you like me to help generate additional test cases for error scenarios?
1128-1140: Add assertions for field discarding behavior.While the test verifies that existing fields are not overwritten, it would be clearer to explicitly assert which fields were discarded and why.
Add assertions to explicitly verify discarded fields:
assert.equal( doc2.toSortedJSON(), '{"content":{"x":1,"y":1},"counter":0,"new":{"k":"v"}}', ); +// Verify discarded fields explicitly +assert.equal(doc2.getRootObject().get('counter').getValue(), 0, 'counter should retain original value'); +assert.deepEqual(doc2.getRootObject().get('content'), { x: 1, y: 1 }, 'content should retain original structure');
1240-1241: Address the TODO comment for byte array and date cases.The comment indicates missing test cases for byte arrays and dates. This gap in testing could lead to issues with these data types in production.
Would you like me to help implement the missing test cases for:
- Proper byte array handling with correct JSON serialization
- Date object handling with ISO string conversion
1254-1254: Remove unnecessary rename in destructuring.The
nameparameter is unnecessarily renamed to itself in the destructuring.Apply this diff to simplify the destructuring:
-for (const { name: name, input, expectedJSON } of testCases) { +for (const { name, input, expectedJSON } of testCases) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/src/client/client.ts (2 hunks)
- packages/sdk/test/integration/document_test.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/client/client.ts
🧰 Additional context used
🪛 Biome
packages/sdk/test/integration/document_test.ts
[error] 1258-1258: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (1)
packages/sdk/test/integration/document_test.ts (1)
1262-1275: 🛠️ Refactor suggestionConsider using a more maintainable type definition approach.
The
DocTypeinterface could be improved for better maintainability.Consider using a mapped type to reduce repetition:
-type DocType = { - tree?: Tree; - text?: Text; - counter?: Counter; - null?: null; - boolean?: boolean; - number?: number; - long?: Long; - object?: { k: string }; - array?: Array<JSONElement>; - bytes?: Uint8Array; - // date: Date; -}; +type TestValue = Tree | Text | Counter | null | boolean | number | Long | { k: string } | Array<JSONElement> | Uint8Array; +type DocType = { + [K in typeof testCases[number]['name']]?: TestValue; +};Likely invalid or redundant comment.
hackerwins
force-pushed
the
initial-root
branch
from
ae0e42e to
d65e904
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/sdk/test/integration/document_test.ts (3)
1146-1180: Consider enhancing concurrent attachment test coverage.While the test effectively validates the basic concurrent attachment scenario, consider adding:
- Assertions to verify document states immediately after each sync operation
- Test cases for more complex concurrent modifications
- Verification of conflict resolution with different types of data
Would you like me to help generate additional test cases to improve coverage?
1262-1275: Consider improving type safety in DocType interface.The current type definition could be enhanced to:
- Use more specific types for array elements
- Add proper JSDoc comments for each type
- Consider using branded types for better type safety
Apply this diff to improve type safety:
type DocType = { tree?: Tree; text?: Text; counter?: Counter; null?: null; boolean?: boolean; number?: number; long?: Long; object?: { k: string }; - array?: Array<JSONElement>; + array?: Array<number | string | boolean | null>; bytes?: Uint8Array; // date: Date; };
1258-1260: Remove unnecessary rename in template literal.The variable name matches the property name, making the rename unnecessary.
Apply this diff to simplify the code:
- const docKey = toDocKey( - `${task.name}-${name}-${new Date().getTime()}`, - ); + const docKey = toDocKey(`${task.name}-${name}-${new Date().getTime()}`);🧰 Tools
🪛 Biome
[error] 1258-1258: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/sdk/src/client/client.ts (2 hunks)
- packages/sdk/test/integration/document_test.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/client/client.ts
🧰 Additional context used
🪛 Biome
packages/sdk/test/integration/document_test.ts
[error] 1258-1258: Useless rename.
Safe fix: Remove the renaming.
(lint/complexity/noUselessRename)
🔇 Additional comments (1)
packages/sdk/test/integration/document_test.ts (1)
1106-1144: LGTM! Well-structured test case for InitialRoot functionality.The test case thoroughly validates:
- Document initialization with initial root data
- Proper handling of Counter and nested objects
- Field overwrite behavior when attaching with existing fields
What this PR does / why we need it?
This PR introduces the
initialRootoption during document attachment, allowing users to set initial values for documents with a predefined structure.With this feature, developers can streamline the setup of their documents, ensuring a consistent initial state without having to check and assign default values manually after attaching the document.
Any background context you want to provide?
For more details regarding the policy related to the
initialRootoption, please refer to the discussion in yorkie-team/yorkie#986.The usage of the
initialRootoption is demonstrated below, showcasing how it differs from the previous implementation:What are the relevant tickets?
Related to #yorkie-team/yorkie#986
Addresses #yorkie-team/yorkie#669, #905
Checklist
Summary by CodeRabbit
New Features
attachmethod to allow users to specify an initial root object when attaching documents.Bug Fixes