-
Notifications
You must be signed in to change notification settings - Fork 549
Cleanup unhydrated tree and tree import code more generally #24740
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
Changes from 3 commits
5db0cd3
2869683
12d39c4
3a7f1fa
c4f854e
be284a1
3101d4f
caf824d
99cfdbd
6d400ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,9 @@ | |
* Licensed under the MIT License. | ||
*/ | ||
|
||
import { assert } from "@fluidframework/core-utils/internal"; | ||
|
||
import type { MinimalMapTreeNodeView } from "../../feature-libraries/index.js"; | ||
import type { FieldKey } from "../schema-stored/index.js"; | ||
|
||
import type { NodeData } from "./types.js"; | ||
|
@@ -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 { | ||
type Next = [ | ||
fields: ExclusiveMapTree["fields"], | ||
sourceFields: MinimalMapTreeNodeView["fields"], | ||
]; | ||
function create( | ||
child: MinimalMapTreeNodeView, | ||
fields: ExclusiveMapTree["fields"], | ||
): ExclusiveMapTree { | ||
return { | ||
type: child.type, | ||
...(child.value !== undefined ? { value: child.value } : {}), | ||
fields, | ||
}; | ||
} | ||
|
||
const rootFields: ExclusiveMapTree["fields"] = new Map(); | ||
const nexts: Next[] = [[rootFields, mapTree.fields]]; | ||
for (let next = nexts.pop(); next !== undefined; next = nexts.pop()) { | ||
const [fields, sourceFields] = next; | ||
for (const [key, field] of sourceFields) { | ||
assert(field.length > 0, "invalid map tree: empty field"); | ||
CraigMacomber marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
newField.push(childClone); | ||
nexts.push([childClone.fields, child.fields]); | ||
} | ||
|
@@ -55,8 +73,5 @@ export function deepCopyMapTree(mapTree: MapTree): ExclusiveMapTree { | |
} | ||
} | ||
|
||
return { | ||
...mapTree, | ||
fields: rootFields, | ||
}; | ||
return create(mapTree, rootFields); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -435,7 +435,7 @@ export interface OptionalFieldEditBuilder<TContent> { | |
|
||
/** | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any docs we can add to this? Alternatively, can we remove the blank comment block? |
||
export interface SequenceFieldEditBuilder<TContent> { | ||
export interface SequenceFieldEditBuilder<TContent, TRemoved = void> { | ||
/** | ||
* Issues a change which inserts the `newContent` at the given `index`. | ||
* @param index - the index at which to insert the `newContent`. | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The unhydrated flex tree implements returning the results from remove. This allows that to be expressed more clearly. |
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ import { unreachableCase, fail } from "@fluidframework/core-utils/internal"; | |
import { UsageError } from "@fluidframework/telemetry-utils/internal"; | ||
|
||
import { | ||
type MapTree, | ||
type TreeFieldStoredSchema, | ||
LeafNodeStoredSchema, | ||
ObjectNodeStoredSchema, | ||
|
@@ -16,6 +15,7 @@ import { | |
type SchemaAndPolicy, | ||
} from "../../core/index.js"; | ||
import { allowsValue } from "../valueUtilities.js"; | ||
import type { MapTreeFieldViewGeneric, MinimalMapTreeNodeView } from "../mapTreeCursor.js"; | ||
|
||
export enum SchemaValidationError { | ||
Field_KindNotInSchemaPolicy, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More general type, so it can apply to unhydrated flex tree nodes in #24739 |
||
schemaAndPolicy: SchemaAndPolicy, | ||
): SchemaValidationError | undefined { | ||
// Validate the schema declared by the node exists | ||
|
@@ -86,7 +86,7 @@ export function isNodeInSchema( | |
return SchemaValidationError.ObjectNode_FieldNotInSchema; | ||
} | ||
} else if (schema instanceof MapNodeStoredSchema) { | ||
for (const field of node.fields.values()) { | ||
for (const [_key, field] of node.fields) { | ||
const fieldInSchemaResult = isFieldInSchema(field, schema.mapFields, schemaAndPolicy); | ||
if (fieldInSchemaResult !== undefined) { | ||
return fieldInSchemaResult; | ||
|
@@ -104,7 +104,7 @@ export function isNodeInSchema( | |
* Deeply checks that the nodes comply with the field schema and included schema. | ||
*/ | ||
export function isFieldInSchema( | ||
childNodes: readonly MapTree[], | ||
childNodes: MapTreeFieldViewGeneric<MinimalMapTreeNodeView>, | ||
schema: TreeFieldStoredSchema, | ||
schemaAndPolicy: SchemaAndPolicy, | ||
): SchemaValidationError | undefined { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,49 +12,113 @@ import { | |
type FieldKey, | ||
type ITreeCursor, | ||
type MapTree, | ||
type NodeData, | ||
aboveRootPlaceholder, | ||
detachedFieldAsKey, | ||
mapCursorField, | ||
rootField, | ||
rootFieldKey, | ||
} from "../core/index.js"; | ||
|
||
import { | ||
type CursorAdapter, | ||
type CursorWithNode, | ||
type Field, | ||
stackTreeFieldCursor, | ||
stackTreeNodeCursor, | ||
} from "./treeCursorUtils.js"; | ||
import type { requireAssignableTo } from "../util/index.js"; | ||
|
||
export interface MapTreeNodeViewGeneric<TNode> extends NodeData { | ||
Josmithr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* The non-empty fields on this node. | ||
*/ | ||
readonly fields: Pick< | ||
ReadonlyMap<FieldKey, MapTreeFieldViewGeneric<TNode>>, | ||
typeof Symbol.iterator | "get" | "size" | "keys" | ||
>; | ||
} | ||
|
||
export type MapTreeFieldViewGeneric<TNode> = Pick< | ||
readonly TNode[], | ||
typeof Symbol.iterator | "length" | ||
>; | ||
|
||
/** | ||
* Like {@link MapTree} but with the minimal properties needed for reading. | ||
*/ | ||
export interface MinimalMapTreeNodeView | ||
extends MapTreeNodeViewGeneric<MinimalMapTreeNodeView> {} | ||
|
||
{ | ||
type _check1 = requireAssignableTo<MapTree, MinimalMapTreeNodeView>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: a quick comment explaining the type checks would probably be good. |
||
type _check2 = requireAssignableTo<MapTree, MapTreeNodeViewGeneric<MapTree>>; | ||
} | ||
|
||
/** | ||
* Returns an {@link ITreeCursorSynchronous} in nodes mode for a single {@link MapTree}. | ||
*/ | ||
export function cursorForMapTreeNode(root: MapTree): CursorWithNode<MapTree> { | ||
return stackTreeNodeCursor(adapter, root); | ||
export function cursorForMapTreeNode<T extends MapTreeNodeViewGeneric<T>>( | ||
root: T, | ||
): CursorWithNode<T> { | ||
// There doesn't seem to be a clean way to get TypeScript to type check this without casting | ||
// without declaring the adapter inside this generic function and needlessly recreating it on every call. | ||
const adapterTyped = adapter as CursorAdapter<MapTreeNodeViewGeneric<T>> as CursorAdapter<T>; | ||
return stackTreeNodeCursor(adapterTyped, root); | ||
} | ||
|
||
export function mapTreeWithField( | ||
children: ExclusiveMapTree[], | ||
key = rootFieldKey, | ||
type = aboveRootPlaceholder, | ||
): MapTree { | ||
return { | ||
type, | ||
fields: mapTreeFieldsWithField(children, key), | ||
}; | ||
} | ||
|
||
export function mapTreeFieldsWithField<T extends readonly unknown[]>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docs? |
||
children: T, | ||
key: FieldKey, | ||
): Map<FieldKey, T> { | ||
return new Map(children.length === 0 ? [] : [[key, children]]); | ||
} | ||
|
||
/** | ||
* Returns an {@link ITreeCursorSynchronous} in fields mode for a MapTree field. | ||
*/ | ||
export function cursorForMapTreeField( | ||
root: readonly MapTree[], | ||
export function cursorForMapTreeField<T extends MapTreeNodeViewGeneric<T>>( | ||
root: readonly T[], | ||
detachedField: DetachedField = rootField, | ||
): CursorWithNode<MapTree> { | ||
): CursorWithNode<T> { | ||
const key = detachedFieldAsKey(detachedField); | ||
return stackTreeFieldCursor( | ||
adapter, | ||
{ | ||
type: aboveRootPlaceholder, | ||
fields: new Map([[key, root]]), | ||
}, | ||
detachedField, | ||
); | ||
const adapterTyped = adapter as unknown as CursorAdapter<T>; | ||
const dummyRoot: MapTreeNodeViewGeneric<T> = { | ||
type: aboveRootPlaceholder, | ||
fields: mapTreeFieldsWithField(root, key), | ||
}; | ||
return stackTreeFieldCursor(adapterTyped, dummyRoot as T, detachedField); | ||
} | ||
|
||
const adapter: CursorAdapter<MapTree> = { | ||
const adapter: CursorAdapter<MinimalMapTreeNodeView> = { | ||
value: (node) => node.value, | ||
type: (node) => node.type, | ||
keysFromNode: (node) => [...node.fields.keys()], // TODO: don't convert this to array here. | ||
getFieldFromNode: (node, key) => node.fields.get(key) ?? [], | ||
getFieldFromNode: (node, key): Field<MinimalMapTreeNodeView> => { | ||
const field = node.fields.get(key) as | ||
| MinimalMapTreeNodeView[] | ||
| Iterable<MinimalMapTreeNodeView> | ||
| undefined; | ||
if (field === undefined) { | ||
return []; | ||
} | ||
if (Array.isArray(field)) { | ||
return field as readonly MinimalMapTreeNodeView[]; | ||
} | ||
// TODO: this copy could be avoided by using Array.at instead of indexing in `Field`, but that requires ES2022. | ||
return [...field] as readonly MinimalMapTreeNodeView[]; | ||
}, | ||
}; | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can now copy from any MinimalMapTreeNodeView: an interface UnhydratedFlexTree nodes start implementing in #24739