diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md new file mode 100644 index 000000000000..fd1a4e3c686c --- /dev/null +++ b/.changeset/wet-hoops-drop.md @@ -0,0 +1,66 @@ +--- +"fluid-framework": minor +"@fluidframework/tree": minor +--- + + +A `@beta` version of `nodeChanged` which includes the list of properties has been added. + +```typescript +const factory = new SchemaFactory("example"); +class Point2d extends factory.object("Point2d", { + x: factory.number, + y: factory.number, +}) {} + +const point = new Point2d({ x: 0, y: 0 }); + +TreeBeta.on(point, "nodeChanged", (data) => { + const changed: ReadonlySet<"x" | "y"> = data.changedProperties; + if (changed.has("x")) { + // ... + } +}); +``` + +The payload of the `nodeChanged` event emitted by SharedTree's `TreeBeta` includes a `changedProperties` property that indicates +which properties of the node changed. + +For object nodes, the list of properties uses the property identifiers defined in the schema, and not the persisted +identifiers (or "stored keys") that can be provided through `FieldProps` when defining a schema. +See the documentation for `FieldProps` for more details about the distinction between "property keys" and "stored keys". + +For map nodes, every key that was added, removed, or updated by a change to the tree is included in the list of properties. + +For array nodes, the set of properties will always be undefined: there is currently no API to get details about changes to an array. + +Object nodes revieve strongly types sets of changed keys, allowing compile time detection of incorrect keys: + +```typescript +TreeBeta.on(point, "nodeChanged", (data) => { + // @ts-expect-error Strong typing for changed properties of object nodes detects incorrect keys: + if (data.changedProperties.has("z")) { + // ... + } +}); +``` + +The existing stable "nodeChanged" event's callback now is given a parameter called `unstable` of type `unknown` which is used to indicate that additional data can be provided there. +This could break existing code using "nodeChanged" in a particularly fragile way. + +```typescript +function f(optional?: number) { + // ... +} +Tree.on(point, "nodeChanged", f); // Bad +``` + +Code like this which is implicitly discarding an optional argument from the function used as the listener will be broken. +It can be fixed by using an inline lambda expression: + +```typescript +function f(optional?: number) { + // ... +} +Tree.on(point, "nodeChanged", () => f()); // Safe +``` diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index d6882c88c5a7..7238a2b10548 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -286,6 +286,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @beta @sealed +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet ? string & keyof TInfo : string>; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -392,7 +397,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -400,7 +405,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -472,12 +477,22 @@ interface TreeArrayNodeBase extends ReadonlyArray< export interface TreeArrayNodeUnsafe> extends TreeArrayNodeBase, InsertableTreeNodeFromImplicitAllowedTypesUnsafe, TreeArrayNode> { } +// @beta @sealed +export const TreeBeta: { + readonly on: , TNode extends TreeNode>(node: TNode, eventName: K, listener: NoInfer[K]>) => () => void; +}; + // @public @sealed export interface TreeChangeEvents { - nodeChanged(): void; + nodeChanged(unstable?: unknown): void; treeChanged(): void; } +// @beta @sealed +export interface TreeChangeEventsBeta extends TreeChangeEvents { + nodeChanged: (data: NodeChangedData & (TNode extends WithType ? Required, "changedProperties">> : unknown)) => void; +} + // @public export type TreeFieldFromImplicitField = TSchema extends FieldSchema ? ApplyKind, Kind, false> : TSchema extends ImplicitAllowedTypes ? TreeNodeFromImplicitAllowedTypes : unknown; @@ -555,7 +570,7 @@ interface TreeNodeSchemaNonClass, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -619,10 +634,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } // (No @packageDocumentation comment for this package) diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 2c400740fcf5..d7b65e514228 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -218,6 +218,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @beta @sealed +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet ? string & keyof TInfo : string>; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -324,7 +329,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -332,7 +337,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -401,12 +406,22 @@ interface TreeArrayNodeBase extends ReadonlyArray< export interface TreeArrayNodeUnsafe> extends TreeArrayNodeBase, InsertableTreeNodeFromImplicitAllowedTypesUnsafe, TreeArrayNode> { } +// @beta @sealed +export const TreeBeta: { + readonly on: , TNode extends TreeNode>(node: TNode, eventName: K, listener: NoInfer[K]>) => () => void; +}; + // @public @sealed export interface TreeChangeEvents { - nodeChanged(): void; + nodeChanged(unstable?: unknown): void; treeChanged(): void; } +// @beta @sealed +export interface TreeChangeEventsBeta extends TreeChangeEvents { + nodeChanged: (data: NodeChangedData & (TNode extends WithType ? Required, "changedProperties">> : unknown)) => void; +} + // @public export type TreeFieldFromImplicitField = TSchema extends FieldSchema ? ApplyKind, Kind, false> : TSchema extends ImplicitAllowedTypes ? TreeNodeFromImplicitAllowedTypes : unknown; @@ -484,7 +499,7 @@ interface TreeNodeSchemaNonClass, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -548,10 +563,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } // (No @packageDocumentation comment for this package) diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index f6b1d73a3726..2deaf1af2f1e 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -324,7 +324,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -332,7 +332,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -403,7 +403,7 @@ export interface TreeArrayNodeUnsafe, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -548,10 +548,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } // (No @packageDocumentation comment for this package) diff --git a/packages/dds/tree/src/index.ts b/packages/dds/tree/src/index.ts index 074ad528925e..6e5e0f3ee0d2 100644 --- a/packages/dds/tree/src/index.ts +++ b/packages/dds/tree/src/index.ts @@ -93,6 +93,7 @@ export { type FieldProps, type InternalTreeNode, type WithType, + type NodeChangedData, // Types not really intended for public use, but used in links. // Can not be moved to internalTypes since doing so causes app code to throw errors like: // Error: src/simple-tree/objectNode.ts:72:1 - (ae-unresolved-link) The @link reference could not be resolved: The package "@fluidframework/tree" does not have an export "TreeNodeApi" @@ -124,6 +125,9 @@ export { test_RecursiveObject, test_RecursiveObject_base, test_RecursiveObjectPojoMode, + // Beta APIs + TreeBeta, + type TreeChangeEventsBeta, // Back to normal types type JsonTreeSchema, type JsonSchemaId, diff --git a/packages/dds/tree/src/simple-tree/api/index.ts b/packages/dds/tree/src/simple-tree/api/index.ts index f064a7076347..31c54c2e5d17 100644 --- a/packages/dds/tree/src/simple-tree/api/index.ts +++ b/packages/dds/tree/src/simple-tree/api/index.ts @@ -26,6 +26,7 @@ export { } from "./schemaCreationUtilities.js"; export { treeNodeApi, type TreeNodeApi } from "./treeNodeApi.js"; export { createFromInsertable, cursorFromInsertable } from "./create.js"; +export { TreeBeta, type NodeChangedData, type TreeChangeEventsBeta } from "./treeApiBeta.js"; // Exporting the schema (RecursiveObject) to test that recursive types are working correctly. // These are `@internal` so they can't be included in the `InternalClassTreeTypes` due to https://github.com/microsoft/rushstack/issues/3639 diff --git a/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts b/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts new file mode 100644 index 000000000000..956bce801e7d --- /dev/null +++ b/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts @@ -0,0 +1,101 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import type { NodeKind, TreeChangeEvents, TreeNode, WithType } from "../core/index.js"; +import { treeNodeApi } from "./treeNodeApi.js"; + +/** + * Data included for {@link TreeChangeEventsBeta.nodeChanged}. + * @sealed @beta + */ +export interface NodeChangedData { + /** + * When the node changed is an object or Map node, this lists all the properties which changed. + * @remarks + * This only includes changes to the node itself (which would trigger {@link TreeChangeEvents.nodeChanged}). + * + * Set to `undefined` when the {@link NodeKind} does not support this feature (currently just ArrayNodes). + * + * When defined, the set should never be empty, since `nodeChanged` will only be triggered when there is a change, and for the supported node types, the only things that can change are properties. + */ + readonly changedProperties?: ReadonlySet< + // For Object nodes, make changedProperties required and strongly typed with the property names from the schema: + TNode extends WithType + ? string & keyof TInfo + : string + >; +} + +/** + * Extensions to {@link TreeChangeEvents} which are not yet stable. + * + * @sealed @beta + */ +export interface TreeChangeEventsBeta + extends TreeChangeEvents { + /** + * Emitted by a node after a batch of changes has been applied to the tree, if any of the changes affected the node. + * + * - Object nodes define a change as being when the value of one of its properties changes (i.e., the property's value is set, including when set to `undefined`). + * + * - Array nodes define a change as when an element is added, removed, moved or replaced. + * + * - Map nodes define a change as when an entry is added, updated, or removed. + * + * @remarks + * This event is not emitted when: + * + * - Properties of a child node change. Notably, updates to an array node or a map node (like adding or removing + * elements/entries) will emit this event on the array/map node itself, but not on the node that contains the + * array/map node as one of its properties. + * + * - The node is moved to a different location in the tree or removed from the tree. + * In this case the event is emitted on the _parent_ node, not the node itself. + * + * For remote edits, this event is not guaranteed to occur in the same order or quantity that it did in + * the client that made the original edit. + * + * When the event is emitted, the tree is guaranteed to be in-schema. + * + * @privateRemarks + * This event occurs whenever the apparent contents of the node instance change, regardless of what caused the change. + * For example, it will fire when the local client reassigns a child, when part of a remote edit is applied to the + * node, or when the node has to be updated due to resolution of a merge conflict + * (for example a previously applied local change might be undone, then reapplied differently or not at all). + * + * TODO: define and document event ordering (ex: bottom up, with nodeChanged before treeChange on each level). + * + * This defines a property which is a function instead of using the method syntax to avoid function bi-variance issues with the input data to the callback. + */ + nodeChanged: ( + data: NodeChangedData & + // For object and Map nodes, make properties specific to them required instead of optional: + (TNode extends WithType + ? Required, "changedProperties">> + : unknown), + ) => void; +} + +/** + * Extensions to {@link Tree} which are not yet stable. + * @sealed @beta + */ +export const TreeBeta = { + /** + * Register an event listener on the given node. + * @param node - The node whose events should be subscribed to. + * @param eventName - Which event to subscribe to. + * @param listener - The callback to trigger for the event. The tree can be read during the callback, but it is invalid to modify the tree during this callback. + * @returns A callback function which will deregister the event. + * This callback should be called only once. + */ + on, TNode extends TreeNode>( + node: TNode, + eventName: K, + listener: NoInfer[K]>, + ): () => void { + return treeNodeApi.on(node, eventName, listener); + }, +} as const; diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index e038945d0233..2f5c0dc0013c 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -44,6 +44,7 @@ import { type TreeChangeEvents, tryGetTreeNodeSchema, } from "../core/index.js"; +import { isObjectNodeSchema } from "../objectNodeTypes.js"; /** * Provides various functions for analyzing {@link TreeNode}s. @@ -170,12 +171,31 @@ export const treeNodeApi: TreeNodeApi = { const kernel = getKernel(node); switch (eventName) { case "nodeChanged": { - return kernel.on("childrenChangedAfterBatch", () => { - listener(); - }); + const nodeSchema = kernel.schema; + if (isObjectNodeSchema(nodeSchema)) { + return kernel.on("childrenChangedAfterBatch", ({ changedFields }) => { + const changedProperties = new Set( + Array.from( + changedFields, + (field) => + nodeSchema.storedKeyToPropertyKey.get(field) ?? + fail(`Could not find stored key '${field}' in schema.`), + ), + ); + listener({ changedProperties }); + }); + } else if (nodeSchema.kind === NodeKind.Array) { + return kernel.on("childrenChangedAfterBatch", () => { + listener({ changedProperties: undefined }); + }); + } else { + return kernel.on("childrenChangedAfterBatch", ({ changedFields }) => { + listener({ changedProperties: changedFields }); + }); + } } case "treeChanged": { - return kernel.on("subtreeChangedAfterBatch", () => listener()); + return kernel.on("subtreeChangedAfterBatch", () => listener({})); } default: throw new UsageError(`No event named ${JSON.stringify(eventName)}.`); diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index 3e90a8c0c4aa..e15d7c5347c2 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -60,6 +60,11 @@ export interface TreeChangeEvents { * * - Map nodes define a change as when an entry is added, updated, or removed. * + * @param unstable - Future versions of this API (such as the one in beta on TreeBeta) may use this argument to provide additional data to the event. + * users of this event should ensure that they do not provide a listener callback which has an optional parameter in this position, since unexpected data might get provided to it. + * This parameter exists to capture this fact in the type system. + * Using an inline lambda expression as the listener callback is a good pattern to avoid cases like this were arguments are added from breaking due to optional arguments. + * * @remarks * This event is not emitted when: * @@ -83,7 +88,7 @@ export interface TreeChangeEvents { * * TODO: define and document event ordering (ex: bottom up, with nodeChanged before treeChange on each level). */ - nodeChanged(): void; + nodeChanged(unstable?: unknown): void; /** * Emitted by a node after a batch of changes has been applied to the tree, when something changed anywhere in the diff --git a/packages/dds/tree/src/simple-tree/core/withType.ts b/packages/dds/tree/src/simple-tree/core/withType.ts index c350e79145db..1ad00472692f 100644 --- a/packages/dds/tree/src/simple-tree/core/withType.ts +++ b/packages/dds/tree/src/simple-tree/core/withType.ts @@ -43,6 +43,7 @@ export const typeSchemaSymbol: unique symbol = Symbol("TreeNode Schema"); * * @typeParam TName - Same as {@link TreeNodeSchema}'s "Name" parameter. * @typeParam TKind - Same as {@link TreeNodeSchema}'s "Kind" parameter. + * @typeParam TInfo - Same as {@link TreeNodeSchema}'s "Info" parameter: format depends on the Kind. * @remarks * Powers {@link TreeNode}'s strong typing setup. * @example Narrow types for overloading based on NodeKind @@ -75,6 +76,7 @@ export const typeSchemaSymbol: unique symbol = Symbol("TreeNode Schema"); export interface WithType< out TName extends string = string, out TKind extends NodeKind = NodeKind, + out TInfo = unknown, > { /** * Type symbol, marking a type in a way to increase type safety via strong type checking. @@ -85,5 +87,5 @@ export interface WithType< /** * Type symbol, marking a type in a way to increase type safety via strong type checking. */ - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } diff --git a/packages/dds/tree/src/simple-tree/index.ts b/packages/dds/tree/src/simple-tree/index.ts index fa275db74645..681653d3a828 100644 --- a/packages/dds/tree/src/simple-tree/index.ts +++ b/packages/dds/tree/src/simple-tree/index.ts @@ -44,6 +44,9 @@ export { type TreeNodeApi, cursorFromInsertable, createFromInsertable, + type NodeChangedData, + TreeBeta, + type TreeChangeEventsBeta, } from "./api/index.js"; export { type NodeFromSchema, diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts index 97056642d411..9ab2ebf8fd03 100644 --- a/packages/dds/tree/src/simple-tree/objectNode.ts +++ b/packages/dds/tree/src/simple-tree/objectNode.ts @@ -76,7 +76,7 @@ export type ObjectFromSchemaRecord< export type TreeObjectNode< T extends RestrictiveReadonlyRecord, TypeName extends string = string, -> = TreeNode & ObjectFromSchemaRecord & WithType; +> = TreeNode & ObjectFromSchemaRecord & WithType; /** * Type utility for determining whether or not an implicit field schema has a default value. diff --git a/packages/dds/tree/src/test/simple-tree/api/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/api/treeApi.spec.ts index c4b6af816478..efe6e4839249 100644 --- a/packages/dds/tree/src/test/simple-tree/api/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/api/treeApi.spec.ts @@ -19,12 +19,14 @@ import { type NodeFromSchema, SchemaFactory, treeNodeApi as Tree, + TreeBeta, type TreeChangeEvents, + type TreeNode, TreeViewConfiguration, } from "../../../simple-tree/index.js"; import { getView } from "../../utils.js"; -import { hydrate } from "../utils.js"; -import { brand } from "../../../util/index.js"; +import { getViewForForkedBranch, hydrate } from "../utils.js"; +import { brand, type areSafelyAssignable, type requireTrue } from "../../../util/index.js"; import { booleanSchema, @@ -767,5 +769,210 @@ describe("treeNodeApi", () => { assert.equal(nodeChanged, true, "'nodeChanged' should have fired"); assert.equal(treeChanged, true, "'treeChanged' should have fired"); }); + + it(`'nodeChanged' includes the names of changed properties (objectNode)`, () => { + const sb = new SchemaFactory("test"); + class TestNode extends sb.object("root", { + prop1: sb.optional(sb.number), + prop2: sb.optional(sb.number), + prop3: sb.optional(sb.number), + }) {} + + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize({ prop1: 1, prop2: 2 }); + const root = view.root; + + // Using property names here instead of string checks that strong typing works. + const eventLog: ReadonlySet<"prop1" | "prop2" | "prop3">[] = []; + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => { + eventLog.push(changedProperties); + }); + + const { forkView, forkCheckout } = getViewForForkedBranch(view); + + // The implementation details of the kinds of changes that can happen inside the tree are not exposed at this layer. + // But since we know them, try to cover all of them. + forkView.root.prop1 = 2; // Replace + forkView.root.prop2 = undefined; // Detach + forkView.root.prop3 = 3; // Attach + + view.checkout.merge(forkCheckout); + + assert.deepEqual(eventLog, [new Set(["prop1", "prop2", "prop3"])]); + }); + + it(`'nodeChanged' strong typing`, () => { + // Check compile time type checking of property names + + const sb = new SchemaFactory("test"); + class ObjectAB extends sb.object("AB", { + A: sb.optional(sb.number), + B: sb.optional(sb.number), + }) {} + + class ObjectBC extends sb.object("BC", { + B: sb.optional(sb.number), + C: sb.optional(sb.number), + }) {} + + class Map1 extends sb.map("Map1", sb.number) {} + + class Array1 extends sb.array("Array1", sb.number) {} + + const ab = new ObjectAB({}); + const bc = new ObjectBC({}); + const map1 = new Map1({}); + const array = new Array1([]); + + TreeBeta.on(ab, "nodeChanged", (data) => { + const x = data.changedProperties; + type _check = requireTrue>>; + }); + + // @ts-expect-error Incorrect variance (using method syntax for "nodeChanged" makes this build when it shouldn't: this is a regression test for that issue) + TreeBeta.on(ab, "nodeChanged", (data: { changedProperties: ReadonlySet<"A"> }) => { + const x = data.changedProperties; + }); + + function oneOf(...items: T): T[number] { + return items[0]; + } + + function out(data: { changedProperties: ReadonlySet }) { + return data.changedProperties; + } + + function outOpt(data: { changedProperties?: ReadonlySet }) { + return data.changedProperties; + } + + // Strong types work + TreeBeta.on(ab, "nodeChanged", out<"A" | "B">); + TreeBeta.on(ab, "nodeChanged", out); + // Weakly typed (general) callback works + TreeBeta.on(ab, "nodeChanged", outOpt); + TreeBeta.on(ab as TreeNode, "nodeChanged", outOpt); + + // @ts-expect-error Check these test utils work + TreeBeta.on(ab, "nodeChanged", out<"A">); + // @ts-expect-error Check these test utils work + TreeBeta.on(ab, "nodeChanged", out<"A", "B", "C">); + // @ts-expect-error Check these test utils work + TreeBeta.on(ab as TreeNode, "nodeChanged", out<"A">); + + // Union cases + + TreeBeta.on(oneOf(ab, bc), "nodeChanged", out<"A" | "B" | "C">); + TreeBeta.on(oneOf(ab, map1), "nodeChanged", out); + // @ts-expect-error Check map is included + TreeBeta.on(oneOf(ab, map1), "nodeChanged", out<"A" | "B">); + + // @ts-expect-error Array makes changedProperties optional + TreeBeta.on(array, "nodeChanged", out); + TreeBeta.on(array, "nodeChanged", outOpt); + }); + + it(`'nodeChanged' strong typing example`, () => { + const factory = new SchemaFactory("example"); + class Point2d extends factory.object("Point2d", { + x: factory.number, + y: factory.number, + }) {} + + const point = new Point2d({ x: 0, y: 0 }); + + TreeBeta.on(point, "nodeChanged", (data) => { + const changed: ReadonlySet<"x" | "y"> = data.changedProperties; + if (changed.has("x")) { + // ... + } + }); + + TreeBeta.on(point, "nodeChanged", (data) => { + // @ts-expect-error Strong typing for changed properties of object nodes detects incorrect keys: + if (data.changedProperties.has("z")) { + // ... + } + }); + }); + + it(`'nodeChanged' includes the names of changed properties (mapNode)`, () => { + const sb = new SchemaFactory("test"); + class TestNode extends sb.map("root", [sb.number]) {} + + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize( + new Map([ + ["key1", 1], + ["key2", 2], + ]), + ); + const root = view.root; + + const eventLog: ReadonlySet[] = []; + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => + eventLog.push(changedProperties), + ); + + const { forkView, forkCheckout } = getViewForForkedBranch(view); + + // The implementation details of the kinds of changes that can happen inside the tree are not exposed at this layer. + // But since we know them, try to cover all of them. + forkView.root.set("key1", 0); // Replace existing key + forkView.root.delete("key2"); // Remove a key + forkView.root.set("key3", 3); // Add new key + + view.checkout.merge(forkCheckout); + + assert.deepEqual(eventLog, [new Set(["key1", "key2", "key3"])]); + }); + + it(`'nodeChanged' does not include the names of changed properties (arrayNode)`, () => { + const sb = new SchemaFactory("test"); + class TestNode extends sb.array("root", [sb.number]) {} + + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize([1, 2]); + const root = view.root; + + const eventLog: (ReadonlySet | undefined)[] = []; + TreeBeta.on(root, "nodeChanged", (data) => eventLog.push(data.changedProperties)); + + const { forkView, forkCheckout } = getViewForForkedBranch(view); + + // The implementation details of the kinds of changes that can happen inside the tree are not exposed at this layer. + // But since we know them, try to cover all of them. + forkView.root.insertAtEnd(3); // Append to array + forkView.root.removeAt(0); // Remove from array + forkView.root.moveRangeToEnd(0, 1); // Move within array + + view.checkout.merge(forkCheckout); + + assert.deepEqual(eventLog, [undefined]); + }); + + it(`'nodeChanged' uses property keys, not stored keys, for the list of changed properties`, () => { + const sb = new SchemaFactory("test"); + class TestNode extends sb.object("root", { + prop1: sb.optional(sb.number, { key: "stored-prop1" }), + }) {} + + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize({ prop1: 1 }); + const root = view.root; + + const eventLog: ReadonlySet[] = []; + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => + eventLog.push(changedProperties), + ); + + const { forkView, forkCheckout } = getViewForForkedBranch(view); + + forkView.root.prop1 = 2; + + view.checkout.merge(forkCheckout); + + assert.deepEqual(eventLog, [new Set(["prop1"])]); + }); }); }); diff --git a/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts b/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts index 19af2c9dbdb4..e4766fea44f0 100644 --- a/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts @@ -178,6 +178,24 @@ describe("Unhydrated nodes", () => { assert.equal(Tree.status(object), TreeStatus.New); }); + it("preserve event subscriptions during hydration - minimal", () => { + const log: unknown[] = []; + const leafObject = new TestLeaf({ value: "value" }); + + Tree.on(leafObject, "nodeChanged", (data) => { + log.push(data); + }); + Tree.on(leafObject, "treeChanged", () => { + log.push("treeChanged"); + }); + + hydrate(TestLeaf, leafObject); + leafObject.value = "new value"; + // Assert that the event fired + // TODO: Eventually the order of events should be documented, and an approach like this can test that they are ordered as documented. + assert.deepEqual(log, [{ changedProperties: new Set(["value"]) }, "treeChanged"]); + }); + it("preserve events after hydration", () => { function registerEvents(node: TreeNode): () => void { let deepEvent = false; diff --git a/packages/dds/tree/src/test/simple-tree/utils.ts b/packages/dds/tree/src/test/simple-tree/utils.ts index 0d1eab580281..c3ce39076a61 100644 --- a/packages/dds/tree/src/test/simple-tree/utils.ts +++ b/packages/dds/tree/src/test/simple-tree/utils.ts @@ -30,6 +30,9 @@ import { // eslint-disable-next-line import/no-internal-modules import { toFlexSchema, toStoredSchema } from "../../simple-tree/toFlexSchema.js"; import { mintRevisionTag, testIdCompressor, testRevisionTagCodec } from "../utils.js"; +import type { ITreeCheckoutFork } from "../../shared-tree/index.js"; +// eslint-disable-next-line import/no-internal-modules +import { SchematizingSimpleTreeView } from "../../shared-tree/schematizingTreeView.js"; import { CheckoutFlexTreeView, createTreeCheckout } from "../../shared-tree/index.js"; /** @@ -144,3 +147,27 @@ export function pretty(arg: unknown): number | string { } return JSON.stringify(arg); } + +/** + * Creates a branch of the input tree view and returns a new tree view for the branch. + * + * @remarks To merge the branch back into the original view after applying changes on the branch view, use + * `.checkout.merge(.checkout)`. + * + * @param originalView - The tree view to branch. + * @returns A new tree view for a branch of the input tree view, and an {@link ITreeCheckoutFork} object that can be + * used to merge the branch back into the original view. + */ +export function getViewForForkedBranch( + originalView: SchematizingSimpleTreeView, +): { forkView: SchematizingSimpleTreeView; forkCheckout: ITreeCheckoutFork } { + const forkCheckout = originalView.checkout.fork(); + return { + forkView: new SchematizingSimpleTreeView( + forkCheckout, + originalView.config, + originalView.nodeKeyManager, + ), + forkCheckout, + }; +} diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md index b0dca17e2a9a..cd656f8d3c8f 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md @@ -674,7 +674,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -682,7 +682,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -775,7 +775,7 @@ export interface TreeArrayNodeUnsafe, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -920,10 +920,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } ``` diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md index 21dc371ef5f3..545570e7103c 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.alpha.api.md @@ -980,7 +980,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -988,7 +988,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -1172,7 +1172,7 @@ export interface TreeArrayNodeUnsafe, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -1317,10 +1317,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } ``` diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md index 2367f9d20564..10557a506cb1 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.legacy.public.api.md @@ -710,7 +710,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -718,7 +718,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -815,7 +815,7 @@ export interface TreeArrayNodeUnsafe, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -960,10 +960,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } ``` diff --git a/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md b/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md index 9512e8b5921e..b4f27f3d1dc5 100644 --- a/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md +++ b/packages/framework/fluid-framework/api-report/fluid-framework.public.api.md @@ -674,7 +674,7 @@ export class SchemaFactory(allowedTypes: T): TreeNodeSchema`>, NodeKind.Array, TreeArrayNode & WithType`>, NodeKind.Array>, Iterable>, true, T>; array(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNode & WithType, NodeKind.Array>, Iterable>, true, T>; - arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array>, { + arrayRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Array, TreeArrayNodeUnsafe & WithType, NodeKind.Array, unknown>, { [Symbol.iterator](): Iterator>; }, false, T>; readonly boolean: TreeNodeSchema<"com.fluidframework.leaf.boolean", NodeKind.Leaf, boolean, boolean>; @@ -682,7 +682,7 @@ export class SchemaFactory; map(allowedTypes: T): TreeNodeSchema`>, NodeKind.Map, TreeMapNode & WithType`>, NodeKind.Map>, MapNodeInsertableData, true, T>; map(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNode & WithType, NodeKind.Map>, MapNodeInsertableData, true, T>; - mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map>, { + mapRecursive>(name: Name, allowedTypes: T): TreeNodeSchemaClass, NodeKind.Map, TreeMapNodeUnsafe & WithType, NodeKind.Map, unknown>, { [Symbol.iterator](): Iterator<[ string, InsertableTreeNodeFromImplicitAllowedTypesUnsafe @@ -775,7 +775,7 @@ export interface TreeArrayNodeUnsafe, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; +export type TreeObjectNode, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecord & WithType; // @public export type TreeObjectNodeUnsafe>, TypeName extends string = string> = TreeNode & ObjectFromSchemaRecordUnsafe & WithType; @@ -920,10 +920,10 @@ export type ValidateRecursiveSchema> = true; // @public @sealed -export interface WithType { +export interface WithType { // @deprecated get [typeNameSymbol](): TName; - get [typeSchemaSymbol](): TreeNodeSchemaClass; + get [typeSchemaSymbol](): TreeNodeSchemaClass; } ```