Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements deep cloning for the Model class without relying on serialization, enabling more efficient memory usage by sharing immutable data (strings, binary buffers, and IDs) between the original and cloned models.
Key Changes:
- Refactored the
fork()method to perform deep cloning without serialization - Added
clone()methods to all node types (ConNode, ValNode, RootNode, ObjNode, VecNode, StrNode, BinNode, ArrNode) - Added comprehensive test coverage for clone functionality including state verification, isolation, and edge cases
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/json-joy/src/json-crdt/model/Model.ts |
Refactored fork() to perform deep clone by iterating through nodes and cloning each based on type |
packages/json-joy/src/json-crdt/nodes/vec/VecNode.ts |
Added clone() method with shallow copy of elements array; reorganized code structure |
packages/json-joy/src/json-crdt/nodes/val/ValNode.ts |
Added clone() method that creates new ValNode with same id and value |
packages/json-joy/src/json-crdt/nodes/str/StrNode.ts |
Added clone() methods for both StrChunk and StrNode with RGA structure preservation |
packages/json-joy/src/json-crdt/nodes/root/RootNode.ts |
Added clone() method that creates new RootNode with same value |
packages/json-joy/src/json-crdt/nodes/obj/ObjNode.ts |
Added clone() method that copies keys map; reorganized code structure |
packages/json-joy/src/json-crdt/nodes/const/ConNode.ts |
Added clone() method for immutable constant nodes; reorganized code structure |
packages/json-joy/src/json-crdt/nodes/bin/BinNode.ts |
Added clone() methods for both BinChunk and BinNode with RGA structure preservation |
packages/json-joy/src/json-crdt/nodes/arr/ArrNode.ts |
Added clone() method that clones RGA chunks with array data; reorganized code structure |
packages/json-joy/src/json-crdt/model/__tests__/Model.cloning.spec.ts |
Added extensive test suite covering string/binary data sharing, clock state, index completeness, API independence, RGA structure preservation, and mutation isolation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * - `id`, `span`, `len`, `del`, `data`: copied, set by constructor | ||
| * - `p`, `l`, `r`, `p2`, `l2`, `r2`, `s`: not copied, set when inserted into RGA | ||
| */ | ||
| public clone(): StrChunk { | ||
| const chunk = new StrChunk(this.id, this.span, this.data); |
There was a problem hiding this comment.
The StrChunk clone method doesn't explicitly preserve the del and len fields. While the constructor will recalculate them based on the data parameter, this could be problematic for deleted chunks. When a chunk is deleted, this.del is true and this.data is an empty string. The constructor with an empty string will set del = false (because empty string is truthy) and len = span, which is incorrect for deleted chunks. Consider explicitly preserving these fields or handling the deleted state properly in the clone method.
| * - `id`, `span`, `len`, `del`, `data`: copied, set by constructor | |
| * - `p`, `l`, `r`, `p2`, `l2`, `r2`, `s`: not copied, set when inserted into RGA | |
| */ | |
| public clone(): StrChunk { | |
| const chunk = new StrChunk(this.id, this.span, this.data); | |
| * - `id`, `span`, `data`: copied via constructor | |
| * - `len`, `del`: explicitly preserved from the source chunk | |
| * - `p`, `l`, `r`, `p2`, `l2`, `r2`, `s`: not copied, set when inserted into RGA | |
| */ | |
| public clone(): StrChunk { | |
| const chunk = new StrChunk(this.id, this.span, this.data); | |
| chunk.del = this.del; | |
| chunk.len = this.len; |
| index.forEach(({v: node}) => { | ||
| let cloned: JsonNode; | ||
| if (node instanceof ConNode) cloned = node.clone(); | ||
| else if (node instanceof ValNode) cloned = node.clone(copy); | ||
| else if (node instanceof ObjNode) cloned = node.clone(copy); | ||
| else if (node instanceof VecNode) cloned = node.clone(copy); | ||
| else if (node instanceof StrNode) cloned = node.clone(); | ||
| else if (node instanceof BinNode) cloned = node.clone(); | ||
| else if (node instanceof ArrNode) cloned = node.clone(copy); | ||
| else throw new Error('UNKNOWN_NODE'); | ||
| copyIndex.set(cloned.id, cloned); | ||
| }); | ||
| copy.root = this.root.clone(copy) as RootNode<N>; |
There was a problem hiding this comment.
The RootNode is being cloned twice in this method. Since RootNode extends ValNode, when iterating through the index at line 495, the root node will match the ValNode instanceof check at line 498 and be cloned as a ValNode. Then at line 507, it's cloned again as a RootNode, overwriting the first clone in the index. While this works, it's inefficient. Consider either skipping the root node during the index iteration or checking for RootNode before ValNode in the instanceof chain.
| doc1.api.set({a: 1}); | ||
| doc1.api.flush(); | ||
| const doc2 = doc1.fork(); | ||
| expect(doc1.tick).toBe(doc2.tick) |
There was a problem hiding this comment.
Missing semicolon at the end of this statement.
| expect(doc1.tick).toBe(doc2.tick) | |
| expect(doc1.tick).toBe(doc2.tick); |
Implements
Modelclone without serialization.