Cleanup unhydrated tree and tree import code more generally #24740
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Pull Request Overview
This PR cleans up and refactors tree import and unhydrated tree code as part of the overall project maintenance. Key changes include updating error message patterns in verbose tree imports, refining type definitions (e.g. for flexible node content), and updating test cases to reflect these changes.
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts | Added tests for various error cases in verbose tree imports with updated error messages. |
| packages/dds/tree/src/test/simple-tree/api/create.spec.ts | Updated test cases to differentiate failure messages for unknown and out-of-schema inputs. |
| packages/dds/tree/src/test/shared-tree/schematizingTreeView.spec.ts | Removed the unused nodeKeyManager parameter from helper function calls. |
| packages/dds/tree/src/simple-tree/toStoredSchema.ts | Refactored type definitions by enforcing readonly maps for field kinds conversion. |
| packages/dds/tree/src/simple-tree/toMapTree.ts | Changed internal helper function names and logic without altering external behavior. |
| packages/dds/tree/src/simple-tree/node-kinds/map/mapNode.ts | Updated the return type for the editor method to reflect flexible node content. |
| packages/dds/tree/src/simple-tree/node-kinds/array/arrayNode.ts | Adjusted private method return types to use flexible field content. |
| packages/dds/tree/src/simple-tree/api/verboseTree.ts & customTree.ts | Updated error handling and introduced an unknown type error helper for consistency. |
| packages/dds/tree/src/feature-libraries | Various updates to type definitions, adapters, and context adjustments to improve type safety. |
| packages/dds/tree/src/core/tree/mapTree.ts | Refactored deep copy logic to improve immutability and added an assertion for empty field checks. |
Comments suppressed due to low confidence (1)
packages/dds/tree/src/test/simple-tree/api/create.spec.ts:29
- [nitpick] The error validation in the 'Failure: unknown schema' test uses validateAssertionError, while other tests use validateUsageError, which might lead to inconsistencies in error handling. Consider reviewing and unifying the expected error type across similar tests.
assert.throws(() => createFromCursor(SchemaFactory.number, cursor), (e: Error) => validateAssertionError(e, /Tree does not conform to schema/),
| if (field.length > 0) { | ||
| const newField: ExclusiveMapTree[] = []; | ||
| for (const child of field) { | ||
| const childClone: ExclusiveMapTree = { ...child, fields: new Map() }; | ||
| const childClone = create(child, new Map()); |
There was a problem hiding this comment.
the use of spread here and below broke when using implementations of where type or value as a getter (not enumerable) or there were other enumerable properties which should not be copied.
spread was used to avoid including the value property when not present, but that has been reimplemented in create above.
| @@ -36,17 +39,32 @@ export interface ExclusiveMapTree extends NodeData, MapTree { | |||
| * @privateRemarks This is implemented iteratively (rather than recursively, which is much simpler) | |||
| * to avoid the possibility of a stack overflow for very deep trees. | |||
| */ | |||
| export function deepCopyMapTree(mapTree: MapTree): ExclusiveMapTree { | |||
| type Next = [fields: ExclusiveMapTree["fields"], sourceFields: MapTree["fields"]]; | |||
| export function deepCopyMapTree(mapTree: MinimalMapTreeNodeView): ExclusiveMapTree { | |||
There was a problem hiding this comment.
This can now copy from any MinimalMapTreeNodeView: an interface UnhydratedFlexTree nodes start implementing in #24739
| @@ -448,7 +448,7 @@ export interface SequenceFieldEditBuilder<TContent> { | |||
| * @param index - The index of the first removed element. | |||
| * @param count - The number of elements to remove. | |||
| */ | |||
| remove(index: number, count: number): void; | |||
| remove(index: number, count: number): TRemoved; | |||
There was a problem hiding this comment.
The unhydrated flex tree implements returning the results from remove. This allows that to be expressed more clearly.
| @@ -45,7 +45,7 @@ export function inSchemaOrThrow(maybeError: SchemaValidationError | undefined): | |||
| * Deeply checks that the provided node complies with the schema based on its identifier. | |||
| */ | |||
| export function isNodeInSchema( | |||
| node: MapTree, | |||
| node: MinimalMapTreeNodeView, | |||
There was a problem hiding this comment.
More general type, so it can apply to unhydrated flex tree nodes in #24739
| @@ -231,3 +232,9 @@ export function replaceHandles<T>(tree: unknown, replacer: HandleConverter<T>): | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| export function unknownTypeError(type: string): never { | |||
There was a problem hiding this comment.
Some new tests for verbose tree show its possible to hit an assert, which has been replaced with this usage error. #24739 ends up adding a second case which needs this same error so its broken out into a reusable function.
| /** | ||
| */ |
There was a problem hiding this comment.
Any docs we can add to this? Alternatively, can we remove the blank comment block?
| extends MapTreeNodeViewGeneric<MinimalMapTreeNodeView> {} | ||
|
|
||
| { | ||
| type _check1 = requireAssignableTo<MapTree, MinimalMapTreeNodeView>; |
There was a problem hiding this comment.
Nit: a quick comment explaining the type checks would probably be good.
| export function mapTreeWithField( | ||
| children: ExclusiveMapTree[], | ||
| key = rootFieldKey, | ||
| type = aboveRootPlaceholder, | ||
| ): MapTree { | ||
| return { | ||
| type, | ||
| fields: mapTreeFieldsWithField(children, key), | ||
| }; | ||
| } | ||
|
|
||
| export function mapTreeFieldsWithField<T extends readonly unknown[]>( |
| @@ -72,6 +72,8 @@ export function stackTreeFieldCursor<TNode>( | |||
| return cursor; | |||
| } | |||
|
|
|||
| export type Field<TNode> = Pick<readonly TNode[], typeof Symbol.iterator | "length" | number>; | |||
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Description
Cleanups split out of #24739
Reviewer Guidance
The review process is outlined on this wiki page.