tree: To map tree work (#21995)

## Description Cleans input type down-casting logic. This removes some dead code, does some refactors, and fixes a couple issues: 1. fields with defaults did not translate the API key to the stored key. This is not called out in the changeset since the only fields with defaults that matter for this are identifiers, and the current API makes it impossible to select a different stored key. 2. the types for implicit node construction of arrays and maps did not match the runtime behavior. This is improved, tested and documented, but not fully fixed.
microsoft · Jul 24, 2024 · 977f96c · 977f96c
1 parent 4c9e611
commit 977f96c
Show file tree

Hide file tree

Showing 29 changed files with 501 additions and 321 deletions.
diff --git a/.changeset/olive-areas-bow.md b/.changeset/olive-areas-bow.md
@@ -0,0 +1,15 @@
+---
+"fluid-framework": minor
+"@fluidframework/tree": minor
+---
+
+Implicit TreeNode construction improvements
+
+ArrayNodes and MapNodes could always be explicitly constructed (using `new`) from iterables.
+The types also allowed using of iterables to construct implicitly construct array nodes and map nodes,
+but this did not work at runtime.
+This has been fixed for all cases except implicitly constructing an ArrayNode form an `Iterable` that is actually a `Map`,
+and implicitly constructing a MapNode from an `Iterable` that is actually an `Array`.
+These cases may be fixed in the future, but require additional work to ensure unions of array nodes and map nodes work correctly.
+
+Additionally MapNodes can now be constructed from `Iterator<readonly [string, content]>` where previously the inner arrays had to be mutable.
diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md
@@ -313,8 +313,8 @@ export class SchemaFactory<out TScope extends string | undefined = string | unde
     readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>;
     readonly handle: TreeNodeSchema<"com.fluidframework.leaf.handle", NodeKind.Leaf, IFluidHandle<unknown>, IFluidHandle<unknown>>;
     get identifier(): FieldSchema<FieldKind.Identifier, typeof SchemaFactory.string>;
-    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
-    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
     mapRecursive<Name extends TName, const T extends Unenforced<ImplicitAllowedTypes>>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNodeUnsafe<T> & WithType<ScopedSchemaName<TScope, Name>>, {
         [Symbol.iterator](): Iterator<[
         string,

diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md
@@ -313,8 +313,8 @@ export class SchemaFactory<out TScope extends string | undefined = string | unde
     readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>;
     readonly handle: TreeNodeSchema<"com.fluidframework.leaf.handle", NodeKind.Leaf, IFluidHandle<unknown>, IFluidHandle<unknown>>;
     get identifier(): FieldSchema<FieldKind.Identifier, typeof SchemaFactory.string>;
-    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
-    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
     mapRecursive<Name extends TName, const T extends Unenforced<ImplicitAllowedTypes>>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNodeUnsafe<T> & WithType<ScopedSchemaName<TScope, Name>>, {
         [Symbol.iterator](): Iterator<[
         string,

diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md
@@ -313,8 +313,8 @@ export class SchemaFactory<out TScope extends string | undefined = string | unde
     readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>;
     readonly handle: TreeNodeSchema<"com.fluidframework.leaf.handle", NodeKind.Leaf, IFluidHandle<unknown>, IFluidHandle<unknown>>;
     get identifier(): FieldSchema<FieldKind.Identifier, typeof SchemaFactory.string>;
-    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
-    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<const T extends TreeNodeSchema | readonly TreeNodeSchema[]>(allowedTypes: T): TreeNodeSchema<ScopedSchemaName<TScope, `Map<${string}>`>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, `Map<${string}>`>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
+    map<Name extends TName, const T extends ImplicitAllowedTypes>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNode<T> & WithType<ScopedSchemaName<TScope, Name>>, Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>, true, T>;
     mapRecursive<Name extends TName, const T extends Unenforced<ImplicitAllowedTypes>>(name: Name, allowedTypes: T): TreeNodeSchemaClass<ScopedSchemaName<TScope, Name>, NodeKind.Map, TreeMapNodeUnsafe<T> & WithType<ScopedSchemaName<TScope, Name>>, {
         [Symbol.iterator](): Iterator<[
         string,

diff --git a/packages/dds/tree/src/core/index.ts b/packages/dds/tree/src/core/index.ts
@@ -96,6 +96,7 @@ export {
 	type DeltaDetachedNodeDestruction,
 	type DeltaDetachedNodeRename,
 	type DeltaFieldChanges,
+	type ExclusiveMapTree,
 } from "./tree/index.js";
 
 export {

diff --git a/packages/dds/tree/src/core/tree/index.ts b/packages/dds/tree/src/core/tree/index.ts
@@ -43,7 +43,7 @@ export type {
 	DetachedNodeRename as DeltaDetachedNodeRename,
 	FieldChanges as DeltaFieldChanges,
 } from "./delta.js";
-export type { MapTree } from "./mapTree.js";
+export type { MapTree, ExclusiveMapTree } from "./mapTree.js";
 export {
 	clonePath,
 	topDownPath,

diff --git a/packages/dds/tree/src/core/tree/mapTree.ts b/packages/dds/tree/src/core/tree/mapTree.ts
@@ -19,3 +19,16 @@ import type { NodeData } from "./types.js";
 export interface MapTree extends NodeData {
 	readonly fields: ReadonlyMap<FieldKey, readonly MapTree[]>;
 }
+
+/**
+ * {@link MapTree} which is owned by a single reference, and allowed to be mutated.
+ *
+ * @remarks
+ * To not keep multiple references to a value with this type around to avoid unexpected mutations.
+ * While this type does implement MapTree, it should not be used as a MapTree while it is being mutated.
+ *
+ * @internal
+ */
+export interface ExclusiveMapTree extends NodeData, MapTree {
+	fields: Map<FieldKey, ExclusiveMapTree[]>;
+}
diff --git a/packages/dds/tree/src/simple-tree/arrayNode.ts b/packages/dds/tree/src/simple-tree/arrayNode.ts
@@ -3,12 +3,7 @@
  * Licensed under the MIT License.
  */
 
-import {
-	CursorLocationType,
-	EmptyKey,
-	type ITreeCursorSynchronous,
-	type TreeNodeSchemaIdentifier,
-} from "../core/index.js";
+import { CursorLocationType, EmptyKey, type ITreeCursorSynchronous } from "../core/index.js";
 import {
 	type FlexAllowedTypes,
 	type FlexFieldNodeSchema,
@@ -24,7 +19,6 @@ import {
 import {
 	type InsertableContent,
 	getOrCreateNodeProxy,
-	markContentType,
 	prepareContentForHydration,
 } from "./proxies.js";
 import { getFlexNode, getKernel } from "./proxyBinding.js";
@@ -939,8 +933,8 @@ export function arraySchema<
 			return getOrCreateMapTreeNode(
 				flexSchema,
 				mapTreeFromNodeData(
-					copyContent(
-						flexSchema.name,
+					// Ensure input iterable is not an map. See TODO in shallowCompatibilityTest.
+					Array.from(
 						input as Iterable<InsertableTreeNodeFromImplicitAllowedTypes<T>>,
 					) as object,
 					this as unknown as ImplicitAllowedTypes,
@@ -996,12 +990,6 @@ export function arraySchema<
 	return schema;
 }
 
-function copyContent<T>(typeName: TreeNodeSchemaIdentifier, content: Iterable<T>): T[] {
-	const copy = Array.from(content);
-	markContentType(typeName, copy);
-	return copy;
-}
-
 function validateSafeInteger(index: number): void {
 	if (!Number.isSafeInteger(index)) {
 		throw new UsageError(`Expected a safe integer, got ${index}.`);

diff --git a/packages/dds/tree/src/simple-tree/index.ts b/packages/dds/tree/src/simple-tree/index.ts
@@ -41,7 +41,7 @@ export {
 export { SchemaFactory, type ScopedSchemaName } from "./schemaFactory.js";
 export { getFlexNode, tryDisposeTreeNode } from "./proxyBinding.js";
 export { treeNodeApi, type TreeNodeApi } from "./treeNodeApi.js";
-export { toFlexSchema, cursorFromUnhydratedRoot } from "./toFlexSchema.js";
+export { toFlexSchema } from "./toFlexSchema.js";
 export type {
 	FieldHasDefaultUnsafe,
 	ObjectFromSchemaRecordUnsafe,

diff --git a/packages/dds/tree/src/simple-tree/mapNode.ts b/packages/dds/tree/src/simple-tree/mapNode.ts
@@ -3,7 +3,6 @@
  * Licensed under the MIT License.
  */
 
-import type { TreeNodeSchemaIdentifier } from "../core/index.js";
 import {
 	type FlexMapNodeSchema,
 	type FlexTreeNode,
@@ -16,7 +15,6 @@ import {
 import {
 	type InsertableContent,
 	getProxyForField,
-	markContentType,
 	prepareContentForHydration,
 } from "./proxies.js";
 import { getFlexNode } from "./proxyBinding.js";
@@ -127,7 +125,7 @@ const handler: ProxyHandler<TreeMapNode> = {
 };
 
 abstract class CustomMapNodeBase<const T extends ImplicitAllowedTypes> extends TreeNodeValid<
-	Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>
+	Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>
 > {
 	public static readonly kind = NodeKind.Map;
 
@@ -240,7 +238,8 @@ export function mapSchema<
 			return getOrCreateMapTreeNode(
 				flexSchema,
 				mapTreeFromNodeData(
-					copyContent(flexSchema.name, input as Iterable<[string, InsertableContent]>),
+					// Ensure input iterable is not an array. See TODO in shallowCompatibilityTest.
+					new Map(input as Iterable<readonly [string, InsertableContent]>),
 					this as unknown as ImplicitAllowedTypes,
 				),
 			);
@@ -265,18 +264,9 @@ export function mapSchema<
 		TName,
 		NodeKind.Map,
 		TreeMapNode<T> & WithType<TName>,
-		Iterable<[string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>,
+		Iterable<readonly [string, InsertableTreeNodeFromImplicitAllowedTypes<T>]>,
 		ImplicitlyConstructable,
 		T
 	> = schema;
 	return schemaErased;
 }
-
-function copyContent<T>(
-	typeName: TreeNodeSchemaIdentifier,
-	content: Iterable<[string, T]>,
-): Map<string, T> {
-	const copy = new Map(content);
-	markContentType(typeName, copy);
-	return copy;
-}
diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts
@@ -5,7 +5,7 @@
 
 import { UsageError } from "@fluidframework/telemetry-utils/internal";
 
-import type { FieldKey, TreeNodeSchemaIdentifier } from "../core/index.js";
+import type { FieldKey } from "../core/index.js";
 import {
 	cursorForMapTreeNode,
 	FieldKinds,
@@ -23,14 +23,12 @@ import {
 import {
 	type InsertableContent,
 	getProxyForField,
-	markContentType,
 	prepareContentForHydration,
 } from "./proxies.js";
 import { getFlexNode } from "./proxyBinding.js";
 import {
 	NodeKind,
 	type ImplicitFieldSchema,
-	type TreeNodeSchemaClass,
 	type WithType,
 	type TreeNodeSchema,
 	getStoredKey,
@@ -52,6 +50,7 @@ import {
 } from "./types.js";
 import { type RestrictiveReadonlyRecord, fail, type FlattenKeys } from "../util/index.js";
 import { getFlexSchema } from "./toFlexSchema.js";
+import type { ObjectNodeSchema, ObjectNodeSchemaInternalData } from "./objectNodeTypes.js";
 
 /**
  * Helper used to produce types for object nodes.
@@ -128,7 +127,10 @@ export type InsertableObjectFromSchemaRecord<
  * Keys with symbols are currently never used, but allowed to make lookups on non-field things
  * (returning undefined) easier.
  */
-type SimpleKeyMap = ReadonlyMap<string | symbol, { storedKey: FieldKey; schema: FieldSchema }>;
+export type SimpleKeyMap = ReadonlyMap<
+	string | symbol,
+	{ storedKey: FieldKey; schema: FieldSchema }
+>;
 
 /**
  * Caches the mappings from view keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}.
@@ -304,7 +306,7 @@ export function objectSchema<
 	identifier: TName,
 	info: T,
 	implicitlyConstructable: ImplicitlyConstructable,
-): ObjectNodeSchema<TName, T, ImplicitlyConstructable> {
+): ObjectNodeSchema<TName, T, ImplicitlyConstructable> & ObjectNodeSchemaInternalData {
 	// Ensure no collisions between final set of view keys, and final set of stored keys (including those
 	// implicitly derived from view keys)
 	assertUniqueKeys(identifier, info);
@@ -320,6 +322,7 @@ export function objectSchema<
 		public static readonly fields: ReadonlyMap<string, FieldSchema> = new Map(
 			[...flexKeyMap].map(([key, value]) => [key as string, value.schema]),
 		);
+		public static readonly flexKeyMap: SimpleKeyMap = flexKeyMap;
 
 		public static override prepareInstance<T2>(
 			this: typeof TreeNodeValid<T2>,
@@ -358,10 +361,7 @@ export function objectSchema<
 		): MapTreeNode {
 			return getOrCreateMapTreeNode(
 				flexSchema,
-				mapTreeFromNodeData(
-					copyContent(flexSchema.name, input as object),
-					this as unknown as ImplicitAllowedTypes,
-				),
+				mapTreeFromNodeData(input as object, this as unknown as ImplicitAllowedTypes),
 			);
 		}
 
@@ -454,43 +454,3 @@ function assertUniqueKeys<
 		derivedStoredKeys.add(storedKey);
 	}
 }
-
-function copyContent<T extends object>(typeName: TreeNodeSchemaIdentifier, content: T): T {
-	const copy = { ...content };
-	markContentType(typeName, copy);
-	return copy;
-}
-
-/**
- * A schema for {@link TreeObjectNode}s.
- * @privateRemarks
- * This is a candidate for being promoted to the public package API.
- */
-export interface ObjectNodeSchema<
-	TName extends string = string,
-	T extends RestrictiveReadonlyRecord<string, ImplicitFieldSchema> = RestrictiveReadonlyRecord<
-		string,
-		ImplicitFieldSchema
-	>,
-	ImplicitlyConstructable extends boolean = boolean,
-> extends TreeNodeSchemaClass<
-		TName,
-		NodeKind.Object,
-		TreeObjectNode<T, TName>,
-		object & InsertableObjectFromSchemaRecord<T>,
-		ImplicitlyConstructable,
-		T
-	> {
-	readonly fields: ReadonlyMap<string, FieldSchema>;
-}
-
-export const ObjectNodeSchema = {
-	// instanceof-based narrowing support for Javascript and TypeScript 5.3 or newer.
-	[Symbol.hasInstance](value: TreeNodeSchema): value is ObjectNodeSchema {
-		return isObjectNodeSchema(value);
-	},
-} as const;
-
-export function isObjectNodeSchema(schema: TreeNodeSchema): schema is ObjectNodeSchema {
-	return schema.kind === NodeKind.Object;
-}
diff --git a/packages/dds/tree/src/simple-tree/objectNodeTypes.ts b/packages/dds/tree/src/simple-tree/objectNodeTypes.ts
@@ -0,0 +1,64 @@
+/*!
+ * Copyright (c) Microsoft Corporation and contributors. All rights reserved.
+ * Licensed under the MIT License.
+ */
+
+import type { RestrictiveReadonlyRecord } from "../util/index.js";
+import type {
+	TreeObjectNode,
+	InsertableObjectFromSchemaRecord,
+	SimpleKeyMap,
+} from "./objectNode.js";
+import {
+	type ImplicitFieldSchema,
+	type TreeNodeSchemaClass,
+	NodeKind,
+	type FieldSchema,
+	type TreeNodeSchema,
+} from "./schemaTypes.js";
+
+/**
+ * A schema for {@link TreeObjectNode}s.
+ * @privateRemarks
+ * This is a candidate for being promoted to the public package API.
+ */
+export interface ObjectNodeSchema<
+	TName extends string = string,
+	T extends RestrictiveReadonlyRecord<string, ImplicitFieldSchema> = RestrictiveReadonlyRecord<
+		string,
+		ImplicitFieldSchema
+	>,
+	ImplicitlyConstructable extends boolean = boolean,
+> extends TreeNodeSchemaClass<
+		TName,
+		NodeKind.Object,
+		TreeObjectNode<T, TName>,
+		object & InsertableObjectFromSchemaRecord<T>,
+		ImplicitlyConstructable,
+		T
+	> {
+	readonly fields: ReadonlyMap<string, FieldSchema>;
+}
+
+/**
+ * Extra data provided on all {@link ObjectNodeSchema} that is not included in the (soon possibly public) ObjectNodeSchema type.
+ */
+export interface ObjectNodeSchemaInternalData {
+	/**
+	 * {@inheritdoc SimpleKeyMap}
+	 */
+	readonly flexKeyMap: SimpleKeyMap;
+}
+
+export const ObjectNodeSchema = {
+	// instanceof-based narrowing support for Javascript and TypeScript 5.3 or newer.
+	[Symbol.hasInstance](value: TreeNodeSchema): value is ObjectNodeSchema {
+		return isObjectNodeSchema(value);
+	},
+} as const;
+
+export function isObjectNodeSchema(
+	schema: TreeNodeSchema,
+): schema is ObjectNodeSchema & ObjectNodeSchemaInternalData {
+	return schema.kind === NodeKind.Object;
+}