From 8577620dc7ac5a3137f8552ca8ea49bab63d38de Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 24 Jul 2024 22:03:46 +0000 Subject: [PATCH 01/34] WIP - test works --- packages/dds/tree/src/core/tree/anchorSet.ts | 23 +++-- .../dds/tree/src/test/tree/anchorSet.spec.ts | 85 +++++++++++++++++++ 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index 6b5839341591..032ff88d345c 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -132,7 +132,7 @@ export interface AnchorEvents { * * Compare to {@link AnchorEvents.childrenChanged} which is emitted in the middle of the batch/delta-visit. */ - childrenChangedAfterBatch(anchor: AnchorNode): void; + childrenChangedAfterBatch(arg: { anchor: AnchorNode, changedFields: Set }): void; /** * Emitted in the middle of applying a batch of changes (i.e. during a delta a visit), if something in the subtree @@ -725,7 +725,7 @@ export class AnchorSet implements Listenable, AnchorLocator pathVisitors: new Map>(), parentField: undefined as FieldKey | undefined, parent: undefined as UpPath | undefined, - bufferedEvents: [] as { node: PathNode; event: keyof AnchorEvents }[], + bufferedEvents: [] as { node: PathNode; event: keyof AnchorEvents, changedField?: FieldKey }[], // 'currentDepth' and 'depthThresholdForSubtreeChanged' serve to keep track of when do we need to emit // subtreeChangedAfterBatch events. @@ -767,7 +767,12 @@ export class AnchorSet implements Listenable, AnchorLocator continue; } emittedEvents?.push(event); - node.events.emit(event, node); + if (event === "childrenChangedAfterBatch") { + const changedFields = new Set(this.bufferedEvents.filter(e => e.node === node && e.event === event).map(e => e.changedField as FieldKey)); + node.events.emit(event, { anchor: node, changedFields }); + } else { + node.events.emit(event, node); + } } }, notifyChildrenChanging(): void { @@ -776,13 +781,14 @@ export class AnchorSet implements Listenable, AnchorLocator () => this.anchorSet.events.emit("childrenChanging", this.anchorSet), ); }, - notifyChildrenChanged(): void { + notifyChildrenChanged(field: FieldKey): void { this.maybeWithNode( (p) => { p.events.emit("childrenChanged", p); this.bufferedEvents.push({ node: p, event: "childrenChangedAfterBatch", + changedField: field }); }, () => {}, @@ -832,7 +838,8 @@ export class AnchorSet implements Listenable, AnchorLocator attach(source: FieldKey, count: number, destination: PlaceIndex): void { this.notifyChildrenChanging(); this.attachEdit(source, count, destination); - this.notifyChildrenChanged(); + assert(this.parentField !== undefined, "Must be in a field in order to attach"); + this.notifyChildrenChanged(this.parentField); }, attachEdit(source: FieldKey, count: number, destination: PlaceIndex): void { assert( @@ -896,7 +903,8 @@ export class AnchorSet implements Listenable, AnchorLocator detach(source: Range, destination: FieldKey): void { this.notifyChildrenChanging(); this.detachEdit(source, destination); - this.notifyChildrenChanged(); + assert(this.parentField !== undefined, "Must be in a field in order to attach"); + this.notifyChildrenChanged(this.parentField); }, detachEdit(source: Range, destination: FieldKey): void { assert( @@ -982,7 +990,8 @@ export class AnchorSet implements Listenable, AnchorLocator this.notifyChildrenChanging(); this.detachEdit(range, oldContentDestination); this.attachEdit(newContentSource, range.end - range.start, range.start); - this.notifyChildrenChanged(); + assert(this.parentField !== undefined, "Must be in a field in order to attach"); + this.notifyChildrenChanged(this.parentField); }, destroy(detachedField: FieldKey, count: number): void { this.anchorSet.removeChildren( diff --git a/packages/dds/tree/src/test/tree/anchorSet.spec.ts b/packages/dds/tree/src/test/tree/anchorSet.spec.ts index dc3a8e59f82b..d93f8a0e3a92 100644 --- a/packages/dds/tree/src/test/tree/anchorSet.spec.ts +++ b/packages/dds/tree/src/test/tree/anchorSet.spec.ts @@ -492,6 +492,91 @@ describe("AnchorSet", () => { ]); }); + it.only("childrenChangedAfterBatch includes the changed fields", () => { + const fieldOne: FieldKey = brand("one"); + const fieldTwo: FieldKey = brand("two"); + + const anchors = new AnchorSet(); + + const anchor0 = anchors.track(makePath([rootFieldKey, 0])); + const node0 = anchors.locate(anchor0) ?? assert.fail(); + + const expectedChangedFields = new Set([fieldOne, fieldTwo]); + let listenerFired = false; + node0.on("childrenChangedAfterBatch", ({ anchor, changedFields }) => { + assert.deepEqual(changedFields, expectedChangedFields); + listenerFired = true; + }); + + const insertMark: DeltaMark = { + count: 1, + attach: buildId, + }; + const detachMark: DeltaMark = { + count: 1, + detach: detachId, + }; + + const detachedNodesToCreate = [ + { + id: buildId, + trees: [cursorForJsonableTreeNode({ type: leaf.string.name, value: "x" })], + }, + ]; + + const insertAtFieldOne = makeFieldDelta( + { + local: [insertMark], + }, + makeFieldPath(fieldOne, [rootFieldKey, 0]), + ); + const removeFromFieldTwo = makeFieldDelta( + { + local: [detachMark], + }, + makeFieldPath(fieldTwo, [rootFieldKey, 0]), + ); + const delta = new Map([ + [ + rootFieldKey, + { + local: [ + { + count: 1, + fields: new Map([ + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + [fieldOne, insertAtFieldOne.get(rootFieldKey)!.local![1].fields!.get(fieldOne)!], + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + [fieldTwo, removeFromFieldTwo.get(rootFieldKey)!.local![1].fields!.get(fieldTwo)!], + ]), + }, + ], + }, + ], + ]); + + // const delta = makeFieldDelta( + // { + // local: [ + // { + // count: 1, + // fields: new Map([ + // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + // [fieldOne, insertAtFieldOne.get(fieldOne)!], + // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + // [fieldTwo, removeFromFieldTwo.get(fieldTwo)!], + // ]), + // }, + // ], + // }, + // rootFieldKey, + // ); + // insertAtFieldOne.get(rootFieldKey)!.local = insertAtFieldOne.get(rootFieldKey)!.local!.concat([{ count: 1 }, detachMark]); + announceTestDelta(delta, anchors, undefined, undefined, detachedNodesToCreate); + + assert.equal(listenerFired, true); + }); + it("triggers path visitor callbacks", () => { const build = [ { From 3cf732b2e15cc250e258136cd1ac2edba4dfbf88 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 24 Jul 2024 22:26:54 +0000 Subject: [PATCH 02/34] Leaner test --- .../dds/tree/src/test/tree/anchorSet.spec.ts | 87 +++++-------------- 1 file changed, 20 insertions(+), 67 deletions(-) diff --git a/packages/dds/tree/src/test/tree/anchorSet.spec.ts b/packages/dds/tree/src/test/tree/anchorSet.spec.ts index d93f8a0e3a92..d71de94daf9e 100644 --- a/packages/dds/tree/src/test/tree/anchorSet.spec.ts +++ b/packages/dds/tree/src/test/tree/anchorSet.spec.ts @@ -492,88 +492,41 @@ describe("AnchorSet", () => { ]); }); - it.only("childrenChangedAfterBatch includes the changed fields", () => { + it.only("childrenChangedAfterBatch event includes the changed fields", () => { const fieldOne: FieldKey = brand("one"); const fieldTwo: FieldKey = brand("two"); + const fieldThree: FieldKey = brand("three"); const anchors = new AnchorSet(); const anchor0 = anchors.track(makePath([rootFieldKey, 0])); const node0 = anchors.locate(anchor0) ?? assert.fail(); - const expectedChangedFields = new Set([fieldOne, fieldTwo]); + const expectedChangedFields = new Set([fieldOne, fieldTwo, fieldThree]); let listenerFired = false; node0.on("childrenChangedAfterBatch", ({ anchor, changedFields }) => { + // This is the main validation of this test assert.deepEqual(changedFields, expectedChangedFields); listenerFired = true; }); - const insertMark: DeltaMark = { - count: 1, - attach: buildId, - }; - const detachMark: DeltaMark = { - count: 1, - detach: detachId, - }; - - const detachedNodesToCreate = [ - { - id: buildId, - trees: [cursorForJsonableTreeNode({ type: leaf.string.name, value: "x" })], - }, - ]; - - const insertAtFieldOne = makeFieldDelta( - { - local: [insertMark], - }, - makeFieldPath(fieldOne, [rootFieldKey, 0]), - ); - const removeFromFieldTwo = makeFieldDelta( - { - local: [detachMark], - }, - makeFieldPath(fieldTwo, [rootFieldKey, 0]), - ); - const delta = new Map([ - [ - rootFieldKey, - { - local: [ - { - count: 1, - fields: new Map([ - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - [fieldOne, insertAtFieldOne.get(rootFieldKey)!.local![1].fields!.get(fieldOne)!], - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - [fieldTwo, removeFromFieldTwo.get(rootFieldKey)!.local![1].fields!.get(fieldTwo)!], - ]), - }, - ], - }, - ], - ]); - - // const delta = makeFieldDelta( - // { - // local: [ - // { - // count: 1, - // fields: new Map([ - // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - // [fieldOne, insertAtFieldOne.get(fieldOne)!], - // // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - // [fieldTwo, removeFromFieldTwo.get(fieldTwo)!], - // ]), - // }, - // ], - // }, - // rootFieldKey, - // ); - // insertAtFieldOne.get(rootFieldKey)!.local = insertAtFieldOne.get(rootFieldKey)!.local!.concat([{ count: 1 }, detachMark]); - announceTestDelta(delta, anchors, undefined, undefined, detachedNodesToCreate); + withVisitor(anchors, (v) => { + v.enterField(rootFieldKey); + v.enterNode(0); + v.enterField(fieldOne); + v.detach({ start: 0, end: 1 }, brand("fakeDetachDestination")); + v.exitField(fieldOne); + v.enterField(fieldTwo); + v.attach(brand("fakeAttachSource"), 1, 0); + v.exitField(fieldTwo); + v.enterField(fieldThree); + v.replace(brand("fakeReplaceSource"), { start: 0, end: 1 }, brand("fakeReplaceDestination")); + v.exitField(fieldThree); + v.exitNode(0); + v.exitField(rootFieldKey); + }); + // Make sure the listener actually fired and validated the changed fields. assert.equal(listenerFired, true); }); From 1e9f71f1989addc95ee80ec1808e3ec120fb5737 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:10:19 +0000 Subject: [PATCH 03/34] Use readonly set and remove only from test --- packages/dds/tree/src/core/tree/anchorSet.ts | 2 +- packages/dds/tree/src/test/tree/anchorSet.spec.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index 032ff88d345c..c74b9af00dd5 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -132,7 +132,7 @@ export interface AnchorEvents { * * Compare to {@link AnchorEvents.childrenChanged} which is emitted in the middle of the batch/delta-visit. */ - childrenChangedAfterBatch(arg: { anchor: AnchorNode, changedFields: Set }): void; + childrenChangedAfterBatch(arg: { anchor: AnchorNode, changedFields: ReadonlySet }): void; /** * Emitted in the middle of applying a batch of changes (i.e. during a delta a visit), if something in the subtree diff --git a/packages/dds/tree/src/test/tree/anchorSet.spec.ts b/packages/dds/tree/src/test/tree/anchorSet.spec.ts index d71de94daf9e..0d1bbd529c7d 100644 --- a/packages/dds/tree/src/test/tree/anchorSet.spec.ts +++ b/packages/dds/tree/src/test/tree/anchorSet.spec.ts @@ -492,7 +492,7 @@ describe("AnchorSet", () => { ]); }); - it.only("childrenChangedAfterBatch event includes the changed fields", () => { + it("childrenChangedAfterBatch event includes the changed fields", () => { const fieldOne: FieldKey = brand("one"); const fieldTwo: FieldKey = brand("two"); const fieldThree: FieldKey = brand("three"); @@ -510,6 +510,7 @@ describe("AnchorSet", () => { listenerFired = true; }); + // Try to test all cases of changes happening on a delta visitor: attaches, detaches, replaces withVisitor(anchors, (v) => { v.enterField(rootFieldKey); v.enterNode(0); From dc87e8dfaf579d1efab0b412a1b77994300f4198 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:10:49 +0000 Subject: [PATCH 04/34] Expose changed properties in simple-tree --- .../tree/src/simple-tree/treeNodeKernel.ts | 4 +- packages/dds/tree/src/simple-tree/types.ts | 2 +- .../tree/src/test/simple-tree/treeApi.spec.ts | 46 +++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index fa8df9470385..2cb609de7062 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -31,8 +31,8 @@ export class TreeNodeKernel implements Listenable { public constructor(public readonly node: TreeNode) {} public hydrate(anchorNode: AnchorNode): void { - const offChildrenChanged = anchorNode.on("childrenChangedAfterBatch", () => { - this.#events.emit("nodeChanged"); + const offChildrenChanged = anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { + this.#events.emit("nodeChanged", { changedProperties: changedFields }); }); const offSubtreeChanged = anchorNode.on("subtreeChangedAfterBatch", () => { diff --git a/packages/dds/tree/src/simple-tree/types.ts b/packages/dds/tree/src/simple-tree/types.ts index 749f2569d916..3bf0e4883673 100644 --- a/packages/dds/tree/src/simple-tree/types.ts +++ b/packages/dds/tree/src/simple-tree/types.ts @@ -92,7 +92,7 @@ export interface TreeChangeEvents { * 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). */ - nodeChanged(): void; + nodeChanged({ changedProperties }: { changedProperties: ReadonlySet }): 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/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index d7ad0976eaa8..b28d6f0b21d1 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -768,5 +768,51 @@ describe("treeNodeApi", () => { assert.equal(nodeChanged, true, "'nodeChanged' should have fired"); assert.equal(treeChanged, true, "'treeChanged' should have fired"); }); + + it.only(`'nodeChanged' includes the names of changed properties`, () => { + const sb = new SchemaFactory("test"); + class treeSchema 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: treeSchema })); + view.initialize({ prop1: 1, prop2: 2, prop3: 0 }); + + let listenerFired = false; + Tree.on(view.root, "nodeChanged", ({ changedProperties }) => { + // This is the main validation for the test + assert.deepEqual(changedProperties, new Set(["prop1", "prop2", "prop3"])); + listenerFired = true; + }); + + const branch = view.checkout.fork(); + const rootNode: UpPath = { + parent: undefined, + parentField: rootFieldKey, + parentIndex: 0, + }; + // Replace on prop 1 + branch.editor + .valueField({ parent: rootNode, field: brand("prop1") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 2 })); + // Detach on prop 2 + branch.editor + .valueField({ parent: rootNode, field: brand("prop2") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: undefined })); + // Attach on prop 3 + branch.editor + .valueField({ parent: rootNode, field: brand("prop3") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 3 })); + + view.checkout.merge(branch); + + // Validate changes actually took place and all listeners fired + assert.equal(view.root.prop1, 2, "'prop1' value did not change as expected"); + assert.equal(view.root.prop2, undefined, "'prop2' value did not change as expected"); + assert.equal(view.root.prop3, 3, "'prop3' value did not change as expected"); + assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + }); }); }); From 014288fbc67fa526d45720fe422b894836fe38c2 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Thu, 25 Jul 2024 18:39:23 +0000 Subject: [PATCH 05/34] Fix test at simple-tree layer --- .../tree/src/test/simple-tree/treeApi.spec.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index b28d6f0b21d1..69e2938f166a 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -778,7 +778,7 @@ describe("treeNodeApi", () => { }) {} const view = getView(new TreeViewConfiguration({ schema: treeSchema })); - view.initialize({ prop1: 1, prop2: 2, prop3: 0 }); + view.initialize({ prop1: 1, prop2: 2 /* , prop3: 0 */ }); let listenerFired = false; Tree.on(view.root, "nodeChanged", ({ changedProperties }) => { @@ -793,18 +793,22 @@ describe("treeNodeApi", () => { parentField: rootFieldKey, parentIndex: 0, }; + + // TODO: look at prod code on how we create a view, and do it here based on the branch so I don't have to + // do things through lower-level APIs + // Replace on prop 1 branch.editor - .valueField({ parent: rootNode, field: brand("prop1") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 2 })); + .optionalField({ parent: rootNode, field: brand("prop1") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 2 }), false); // Detach on prop 2 branch.editor - .valueField({ parent: rootNode, field: brand("prop2") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: undefined })); + .optionalField({ parent: rootNode, field: brand("prop2") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: undefined }), false); // Attach on prop 3 branch.editor - .valueField({ parent: rootNode, field: brand("prop3") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 3 })); + .optionalField({ parent: rootNode, field: brand("prop3") }) + .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 3 }), true); view.checkout.merge(branch); From 6d477620efddfca51b7b90249e6dc4b2669a32f1 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Fri, 26 Jul 2024 23:20:13 +0000 Subject: [PATCH 06/34] Formatting and API reports --- .../dds/tree/api-report/tree.alpha.api.md | 4 +++- packages/dds/tree/api-report/tree.beta.api.md | 4 +++- .../dds/tree/api-report/tree.public.api.md | 4 +++- packages/dds/tree/src/core/tree/anchorSet.ts | 19 +++++++++++++++---- .../tree/src/simple-tree/treeNodeKernel.ts | 9 ++++++--- .../dds/tree/src/test/tree/anchorSet.spec.ts | 6 +++++- 6 files changed, 35 insertions(+), 11 deletions(-) diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 2bf794781f6a..c3adc71c48f5 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -386,7 +386,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index fcf846927f9c..8e1402042e94 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -386,7 +386,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index 088e011ed419..5e6c99b70ea5 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -386,7 +386,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index c74b9af00dd5..a02107e7e687 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -132,7 +132,10 @@ export interface AnchorEvents { * * Compare to {@link AnchorEvents.childrenChanged} which is emitted in the middle of the batch/delta-visit. */ - childrenChangedAfterBatch(arg: { anchor: AnchorNode, changedFields: ReadonlySet }): void; + childrenChangedAfterBatch(arg: { + anchor: AnchorNode; + changedFields: ReadonlySet; + }): void; /** * Emitted in the middle of applying a batch of changes (i.e. during a delta a visit), if something in the subtree @@ -725,7 +728,11 @@ export class AnchorSet implements Listenable, AnchorLocator pathVisitors: new Map>(), parentField: undefined as FieldKey | undefined, parent: undefined as UpPath | undefined, - bufferedEvents: [] as { node: PathNode; event: keyof AnchorEvents, changedField?: FieldKey }[], + bufferedEvents: [] as { + node: PathNode; + event: keyof AnchorEvents; + changedField?: FieldKey; + }[], // 'currentDepth' and 'depthThresholdForSubtreeChanged' serve to keep track of when do we need to emit // subtreeChangedAfterBatch events. @@ -768,7 +775,11 @@ export class AnchorSet implements Listenable, AnchorLocator } emittedEvents?.push(event); if (event === "childrenChangedAfterBatch") { - const changedFields = new Set(this.bufferedEvents.filter(e => e.node === node && e.event === event).map(e => e.changedField as FieldKey)); + const changedFields = new Set( + this.bufferedEvents + .filter((e) => e.node === node && e.event === event) + .map((e) => e.changedField as FieldKey), + ); node.events.emit(event, { anchor: node, changedFields }); } else { node.events.emit(event, node); @@ -788,7 +799,7 @@ export class AnchorSet implements Listenable, AnchorLocator this.bufferedEvents.push({ node: p, event: "childrenChangedAfterBatch", - changedField: field + changedField: field, }); }, () => {}, diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index 2cb609de7062..a28ac987ca0d 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -31,9 +31,12 @@ export class TreeNodeKernel implements Listenable { public constructor(public readonly node: TreeNode) {} public hydrate(anchorNode: AnchorNode): void { - const offChildrenChanged = anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { - this.#events.emit("nodeChanged", { changedProperties: changedFields }); - }); + const offChildrenChanged = anchorNode.on( + "childrenChangedAfterBatch", + ({ changedFields }) => { + this.#events.emit("nodeChanged", { changedProperties: changedFields }); + }, + ); const offSubtreeChanged = anchorNode.on("subtreeChangedAfterBatch", () => { this.#events.emit("treeChanged"); diff --git a/packages/dds/tree/src/test/tree/anchorSet.spec.ts b/packages/dds/tree/src/test/tree/anchorSet.spec.ts index 0d1bbd529c7d..01ae1b577cef 100644 --- a/packages/dds/tree/src/test/tree/anchorSet.spec.ts +++ b/packages/dds/tree/src/test/tree/anchorSet.spec.ts @@ -521,7 +521,11 @@ describe("AnchorSet", () => { v.attach(brand("fakeAttachSource"), 1, 0); v.exitField(fieldTwo); v.enterField(fieldThree); - v.replace(brand("fakeReplaceSource"), { start: 0, end: 1 }, brand("fakeReplaceDestination")); + v.replace( + brand("fakeReplaceSource"), + { start: 0, end: 1 }, + brand("fakeReplaceDestination"), + ); v.exitField(fieldThree); v.exitNode(0); v.exitField(rootFieldKey); From 97632407a26d22d9b54f2fe7be01c26fe602e456 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Fri, 26 Jul 2024 23:21:43 +0000 Subject: [PATCH 07/34] Write simple-tree test in terms of view operations, not editor operations --- packages/dds/tree/src/shared-tree/index.ts | 2 + .../tree/src/test/simple-tree/treeApi.spec.ts | 42 +++++++------------ .../dds/tree/src/test/simple-tree/utils.ts | 20 +++++++++ 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/packages/dds/tree/src/shared-tree/index.ts b/packages/dds/tree/src/shared-tree/index.ts index 898c515b1c1b..cc1f45d56139 100644 --- a/packages/dds/tree/src/shared-tree/index.ts +++ b/packages/dds/tree/src/shared-tree/index.ts @@ -33,6 +33,8 @@ export { buildTreeConfiguration, } from "./schematizeTree.js"; +export { SchematizingSimpleTreeView } from "./schematizingTreeView.js"; + export { type FlexTreeView, CheckoutFlexTreeView, diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 69e2938f166a..742d5d2a2168 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -23,7 +23,7 @@ import { TreeViewConfiguration, } from "../../simple-tree/index.js"; import { getView } from "../utils.js"; -import { hydrate } from "./utils.js"; +import { getViewForForkedBranch, hydrate } from "./utils.js"; import { brand } from "../../util/index.js"; import { leaf } from "../../domains/index.js"; @@ -769,7 +769,7 @@ describe("treeNodeApi", () => { assert.equal(treeChanged, true, "'treeChanged' should have fired"); }); - it.only(`'nodeChanged' includes the names of changed properties`, () => { + it(`'nodeChanged' includes the names of changed properties`, () => { const sb = new SchemaFactory("test"); class treeSchema extends sb.object("root", { prop1: sb.optional(sb.number), @@ -779,43 +779,29 @@ describe("treeNodeApi", () => { const view = getView(new TreeViewConfiguration({ schema: treeSchema })); view.initialize({ prop1: 1, prop2: 2 /* , prop3: 0 */ }); + const root = view.root; let listenerFired = false; - Tree.on(view.root, "nodeChanged", ({ changedProperties }) => { + Tree.on(root, "nodeChanged", ({ changedProperties }) => { // This is the main validation for the test assert.deepEqual(changedProperties, new Set(["prop1", "prop2", "prop3"])); listenerFired = true; }); - const branch = view.checkout.fork(); - const rootNode: UpPath = { - parent: undefined, - parentField: rootFieldKey, - parentIndex: 0, - }; + const branchView = getViewForForkedBranch(view); - // TODO: look at prod code on how we create a view, and do it here based on the branch so I don't have to - // do things through lower-level APIs - - // Replace on prop 1 - branch.editor - .optionalField({ parent: rootNode, field: brand("prop1") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 2 }), false); - // Detach on prop 2 - branch.editor - .optionalField({ parent: rootNode, field: brand("prop2") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: undefined }), false); - // Attach on prop 3 - branch.editor - .optionalField({ parent: rootNode, field: brand("prop3") }) - .set(cursorForJsonableTreeNode({ type: leaf.number.name, value: 3 }), true); + // The implementation details of the kinds of changes that can happen inside the tree are not exposed at this layer. + // But since we know then, try to cover all of them. + branchView.root.prop1 = 2; // Replace + branchView.root.prop2 = undefined; // Detach + branchView.root.prop3 = 3; // Attach - view.checkout.merge(branch); + view.checkout.merge(branchView.checkout); // Validate changes actually took place and all listeners fired - assert.equal(view.root.prop1, 2, "'prop1' value did not change as expected"); - assert.equal(view.root.prop2, undefined, "'prop2' value did not change as expected"); - assert.equal(view.root.prop3, 3, "'prop3' value did not change as expected"); + assert.equal(root.prop1, 2, "'prop1' value did not change as expected"); + assert.equal(root.prop2, undefined, "'prop2' value did not change as expected"); + assert.equal(root.prop3, 3, "'prop3' value did not change as expected"); assert.equal(listenerFired, true, "'nodeChanged' should have fired"); }); }); diff --git a/packages/dds/tree/src/test/simple-tree/utils.ts b/packages/dds/tree/src/test/simple-tree/utils.ts index 7a417bfeffd1..c07fa999f627 100644 --- a/packages/dds/tree/src/test/simple-tree/utils.ts +++ b/packages/dds/tree/src/test/simple-tree/utils.ts @@ -10,6 +10,7 @@ import { getSchemaAndPolicy, type NodeKeyManager, } from "../../feature-libraries/index.js"; +import { SchematizingSimpleTreeView } from "../../shared-tree/index.js"; import { mapTreeFromNodeData, normalizeFieldSchema, @@ -63,3 +64,22 @@ 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. + */ +export function getViewForForkedBranch( + originalView: SchematizingSimpleTreeView, +): SchematizingSimpleTreeView { + return new SchematizingSimpleTreeView( + originalView.checkout.fork(), + originalView.config, + originalView.nodeKeyManager, + ); +} From b0d072eb3c88bd4ff9735bfdfb968c2d340f91ec Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Mon, 29 Jul 2024 14:35:46 +0000 Subject: [PATCH 08/34] Fluid framework API reports --- .../fluid-framework/api-report/fluid-framework.beta.api.md | 4 +++- .../api-report/fluid-framework.legacy.alpha.api.md | 4 +++- .../api-report/fluid-framework.legacy.public.api.md | 4 +++- .../fluid-framework/api-report/fluid-framework.public.api.md | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) 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 3b6eb4a69629..c14c3e8b4dea 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 @@ -751,7 +751,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } 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 124edf93e56a..ed4f0107b0e9 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 @@ -1161,7 +1161,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } 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 1140c228aee3..1c568d31cb4c 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 @@ -785,7 +785,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } 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 1505c4a10147..54080870efa7 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 @@ -751,7 +751,9 @@ export interface TreeArrayNodeUnsafe; + }): void; treeChanged(): void; } From 4477a8bec410d85a885fe339bf7ea36bf5d71030 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Mon, 29 Jul 2024 15:35:13 +0000 Subject: [PATCH 09/34] Test map and array nodes --- packages/dds/tree/src/core/tree/anchorSet.ts | 13 ++-- .../tree/src/test/simple-tree/treeApi.spec.ts | 64 ++++++++++++++++++- 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index a02107e7e687..a88e99bb0be7 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -775,11 +775,14 @@ export class AnchorSet implements Listenable, AnchorLocator } emittedEvents?.push(event); if (event === "childrenChangedAfterBatch") { - const changedFields = new Set( - this.bufferedEvents - .filter((e) => e.node === node && e.event === event) - .map((e) => e.changedField as FieldKey), - ); + const fieldKeys = this.bufferedEvents + .filter((e) => e.node === node && e.event === event) + .map((e) => e.changedField as FieldKey); + // For array nodes, the field key will always be an empty string. + // In that case we want to emit the event with an empty set, instead of a set with the empty string as only member. + const changedFields = fieldKeys.every((x) => x === "") + ? new Set() + : new Set(fieldKeys); node.events.emit(event, { anchor: node, changedFields }); } else { node.events.emit(event, node); diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 742d5d2a2168..32c26d1b59d8 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -769,7 +769,7 @@ describe("treeNodeApi", () => { assert.equal(treeChanged, true, "'treeChanged' should have fired"); }); - it(`'nodeChanged' includes the names of changed properties`, () => { + it(`'nodeChanged' includes the names of changed properties (objectNode)`, () => { const sb = new SchemaFactory("test"); class treeSchema extends sb.object("root", { prop1: sb.optional(sb.number), @@ -804,5 +804,67 @@ describe("treeNodeApi", () => { assert.equal(root.prop3, 3, "'prop3' value did not change as expected"); assert.equal(listenerFired, true, "'nodeChanged' should have fired"); }); + + it(`'nodeChanged' includes the names of changed properties (mapNode)`, () => { + const sb = new SchemaFactory("test"); + class treeSchema extends sb.map("root", [sb.number]) {} + + const view = getView(new TreeViewConfiguration({ schema: treeSchema })); + view.initialize(new Map([["key1", 1], ["key2", 2]])); + const root = view.root; + + let listenerFired = false; + Tree.on(root, "nodeChanged", ({ changedProperties }) => { + // This is the main validation for the test + assert.deepEqual(changedProperties, new Set(["key1", "key2", "key3"])); + listenerFired = true; + }); + + const branchView = 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 then, try to cover all of them. + branchView.root.set("key1", 0); // Replace existing key + branchView.root.delete("key2"); // Remove a key + branchView.root.set("key3", 3); // Add new key + + view.checkout.merge(branchView.checkout); + + // Validate changes actually took place and all listeners fired + assert.equal(root.get("key1"), 0, "'key1' value did not change as expected"); + assert.equal(root.get("key2"), undefined, "'key2' value did not change as expected"); + assert.equal(root.get("key3"), 3, "'key3' value did not change as expected"); + assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + }); + + it(`'nodeChanged' includes the names of changed properties (arrayNode)`, () => { + const sb = new SchemaFactory("test"); + class treeSchema extends sb.array("root", [sb.number]) {} + + const view = getView(new TreeViewConfiguration({ schema: treeSchema })); + view.initialize([1, 2]); + const root = view.root; + + let listenerFired = false; + Tree.on(root, "nodeChanged", ({ changedProperties }) => { + // This is the main validation for the test + assert.deepEqual(changedProperties, new Set()); + listenerFired = true; + }); + + const branchView = 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 then, try to cover all of them. + branchView.root.insertAtEnd(3); // Append to array + branchView.root.removeAt(0); // Remove from arrray + branchView.root.moveRangeToEnd(0, 1); // Move within array + + view.checkout.merge(branchView.checkout); + + // Validate changes actually took place and all listeners fired + assert.deepEqual(Array.from(root), [3, 2]); + assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + }); }); }); From d0b6074345f2d1a4e994a5bdac9a8ecb4cf1a7c8 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Mon, 29 Jul 2024 16:07:10 +0000 Subject: [PATCH 10/34] Add note --- packages/dds/tree/src/test/simple-tree/treeApi.spec.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 32c26d1b59d8..78ff76df8e46 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -863,7 +863,10 @@ describe("treeNodeApi", () => { view.checkout.merge(branchView.checkout); // Validate changes actually took place and all listeners fired - assert.deepEqual(Array.from(root), [3, 2]); + // Note: as of 2024-07-29, if the schema for the root is defined with a name + // (`class treeSchema extends sb.array("root", [sb.number])` where "root" is the name), we need to use Array.from() + // or array spread in order to compare the node to another array. + assert.deepEqual([...root], [3, 2]); assert.equal(listenerFired, true, "'nodeChanged' should have fired"); }); }); From 8e944b697f809bfa8b46da3b7d681a0a8b45b97d Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Mon, 29 Jul 2024 16:29:17 +0000 Subject: [PATCH 11/34] Formatting --- packages/dds/tree/src/test/simple-tree/treeApi.spec.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 78ff76df8e46..91a6c1b87d10 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -810,7 +810,12 @@ describe("treeNodeApi", () => { class treeSchema extends sb.map("root", [sb.number]) {} const view = getView(new TreeViewConfiguration({ schema: treeSchema })); - view.initialize(new Map([["key1", 1], ["key2", 2]])); + view.initialize( + new Map([ + ["key1", 1], + ["key2", 2], + ]), + ); const root = view.root; let listenerFired = false; From 8f0b81e78cb849f698fdbaf96f12c53cca722449 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Tue, 30 Jul 2024 17:12:05 +0000 Subject: [PATCH 12/34] Return forked checkout object --- .../tree/src/test/simple-tree/treeApi.spec.ts | 30 +++++++++---------- .../dds/tree/src/test/simple-tree/utils.ts | 24 ++++++++++----- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 91a6c1b87d10..3822868e116e 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -788,15 +788,15 @@ describe("treeNodeApi", () => { listenerFired = true; }); - const branchView = getViewForForkedBranch(view); + 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 then, try to cover all of them. - branchView.root.prop1 = 2; // Replace - branchView.root.prop2 = undefined; // Detach - branchView.root.prop3 = 3; // Attach + forkView.root.prop1 = 2; // Replace + forkView.root.prop2 = undefined; // Detach + forkView.root.prop3 = 3; // Attach - view.checkout.merge(branchView.checkout); + view.checkout.merge(forkCheckout); // Validate changes actually took place and all listeners fired assert.equal(root.prop1, 2, "'prop1' value did not change as expected"); @@ -825,15 +825,15 @@ describe("treeNodeApi", () => { listenerFired = true; }); - const branchView = getViewForForkedBranch(view); + 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 then, try to cover all of them. - branchView.root.set("key1", 0); // Replace existing key - branchView.root.delete("key2"); // Remove a key - branchView.root.set("key3", 3); // Add new key + 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(branchView.checkout); + view.checkout.merge(forkCheckout); // Validate changes actually took place and all listeners fired assert.equal(root.get("key1"), 0, "'key1' value did not change as expected"); @@ -857,15 +857,15 @@ describe("treeNodeApi", () => { listenerFired = true; }); - const branchView = getViewForForkedBranch(view); + 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 then, try to cover all of them. - branchView.root.insertAtEnd(3); // Append to array - branchView.root.removeAt(0); // Remove from arrray - branchView.root.moveRangeToEnd(0, 1); // Move within array + forkView.root.insertAtEnd(3); // Append to array + forkView.root.removeAt(0); // Remove from arrray + forkView.root.moveRangeToEnd(0, 1); // Move within array - view.checkout.merge(branchView.checkout); + view.checkout.merge(forkCheckout); // Validate changes actually took place and all listeners fired // Note: as of 2024-07-29, if the schema for the root is defined with a name diff --git a/packages/dds/tree/src/test/simple-tree/utils.ts b/packages/dds/tree/src/test/simple-tree/utils.ts index c07fa999f627..09ef1f8c7da9 100644 --- a/packages/dds/tree/src/test/simple-tree/utils.ts +++ b/packages/dds/tree/src/test/simple-tree/utils.ts @@ -10,7 +10,10 @@ import { getSchemaAndPolicy, type NodeKeyManager, } from "../../feature-libraries/index.js"; -import { SchematizingSimpleTreeView } from "../../shared-tree/index.js"; +import { + SchematizingSimpleTreeView, + type ITreeCheckoutFork, +} from "../../shared-tree/index.js"; import { mapTreeFromNodeData, normalizeFieldSchema, @@ -72,14 +75,19 @@ export function pretty(arg: unknown): number | string { * `.checkout.merge(.checkout)`. * * @param originalView - The tree view to branch. - * @returns A new tree view for a branch of the input tree view. + * @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, -): SchematizingSimpleTreeView { - return new SchematizingSimpleTreeView( - originalView.checkout.fork(), - originalView.config, - originalView.nodeKeyManager, - ); +): { forkView: SchematizingSimpleTreeView; forkCheckout: ITreeCheckoutFork } { + const forkCheckout = originalView.checkout.fork(); + return { + forkView: new SchematizingSimpleTreeView( + forkCheckout, + originalView.config, + originalView.nodeKeyManager, + ), + forkCheckout, + }; } From 6c6025f8236c3aeab244a1bd335d02717822b9c9 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Tue, 30 Jul 2024 23:04:21 +0000 Subject: [PATCH 13/34] Translate to view keys --- .../dds/tree/src/simple-tree/objectNode.ts | 39 +++++++++++++------ .../tree/src/simple-tree/objectNodeTypes.ts | 5 +++ .../tree/src/simple-tree/treeNodeKernel.ts | 16 +++++++- .../tree/src/test/simple-tree/treeApi.spec.ts | 36 +++++++++++++++++ 4 files changed, 84 insertions(+), 12 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts index 3708909fbd07..67a4a4ccad41 100644 --- a/packages/dds/tree/src/simple-tree/objectNode.ts +++ b/packages/dds/tree/src/simple-tree/objectNode.ts @@ -133,16 +133,31 @@ export type SimpleKeyMap = ReadonlyMap< >; /** - * Caches the mappings from view keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}. + * Maps from stored field keys to the corresponding "view" keys. + * + * @remarks + * A missing entry for a given stored key indicates that no such field exists in the stored schema. + */ +export type StoredKeyToViewKeyMap = ReadonlyMap; + +/** + * Returns two mappings created from the specified fields: from view keys to stored keys + schema, and from stored keys + * to view keys. */ -function createFlexKeyMapping(fields: Record): SimpleKeyMap { - const keyMap: Map = new Map(); +function createFlexKeyMappings(fields: Record): { + viewKeyMap: SimpleKeyMap; + storedKeyToViewKeyMap: StoredKeyToViewKeyMap; +} { + const viewKeyMap: Map = + new Map(); + const storedKeyToViewKeyMap: Map = new Map(); for (const [viewKey, fieldSchema] of Object.entries(fields)) { const storedKey = getStoredKey(viewKey, fieldSchema); - keyMap.set(viewKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) }); + viewKeyMap.set(viewKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) }); + storedKeyToViewKeyMap.set(storedKey, viewKey); } - return keyMap; + return { viewKeyMap, storedKeyToViewKeyMap }; } /** @@ -311,8 +326,8 @@ export function objectSchema< // implicitly derived from view keys) assertUniqueKeys(identifier, info); - // Performance optimization: cache view key => stored key and schema. - const flexKeyMap: SimpleKeyMap = createFlexKeyMapping(info); + // Performance optimization: cache view key => stored key and schema, and stored key => view key. + const { viewKeyMap, storedKeyToViewKeyMap } = createFlexKeyMappings(info); let handler: ProxyHandler; let customizable: boolean; @@ -320,9 +335,11 @@ export function objectSchema< class CustomObjectNode extends CustomObjectNodeBase { public static readonly fields: ReadonlyMap = new Map( - [...flexKeyMap].map(([key, value]) => [key as string, value.schema]), + [...viewKeyMap].map(([key, value]) => [key as string, value.schema]), ); - public static readonly flexKeyMap: SimpleKeyMap = flexKeyMap; + public static readonly flexKeyMap: SimpleKeyMap = viewKeyMap; + public static readonly storedKeyToViewKeyMap: StoredKeyToViewKeyMap = + storedKeyToViewKeyMap; public static override prepareInstance( this: typeof TreeNodeValid, @@ -370,7 +387,7 @@ export function objectSchema< protected static override oneTimeSetup(this: typeof TreeNodeValid): void { // One time initialization that required knowing the most derived type (from this.constructor) and thus has to be lazy. customizable = (this as unknown) !== CustomObjectNode; - handler = createProxyHandler(flexKeyMap, customizable); + handler = createProxyHandler(viewKeyMap, customizable); flexSchema = getFlexSchema(this as unknown as TreeNodeSchema) as FlexObjectNodeSchema; // First run, do extra validation. @@ -380,7 +397,7 @@ export function objectSchema< let prototype: object = this.prototype; // There isn't a clear cleaner way to author this loop. while (prototype !== CustomObjectNode.prototype) { - for (const [key] of flexKeyMap) { + for (const [key] of viewKeyMap) { if ( // constructor is a special case, since one is built in on the derived type, and shadowing it works fine since we only use it before fields are applied. key !== "constructor" && diff --git a/packages/dds/tree/src/simple-tree/objectNodeTypes.ts b/packages/dds/tree/src/simple-tree/objectNodeTypes.ts index 2d431cc62983..ccee8c4858b1 100644 --- a/packages/dds/tree/src/simple-tree/objectNodeTypes.ts +++ b/packages/dds/tree/src/simple-tree/objectNodeTypes.ts @@ -8,6 +8,7 @@ import type { TreeObjectNode, InsertableObjectFromSchemaRecord, SimpleKeyMap, + StoredKeyToViewKeyMap, } from "./objectNode.js"; import { type ImplicitFieldSchema, @@ -48,6 +49,10 @@ export interface ObjectNodeSchemaInternalData { * {@inheritdoc SimpleKeyMap} */ readonly flexKeyMap: SimpleKeyMap; + /** + * {@inheritdoc StoredKeyToViewKeyMap} + */ + readonly storedKeyToViewKeyMap: StoredKeyToViewKeyMap; } export const ObjectNodeSchema = { diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index a28ac987ca0d..645e37722ca2 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -14,6 +14,10 @@ import { TreeStatus, treeStatusFromAnchorCache, } from "../feature-libraries/index.js"; +import { getFlexNode } from "./index.js"; +import { getSimpleNodeSchema } from "./schemaCaching.js"; +import { fail } from "../util/index.js"; +import { isObjectNodeSchema } from "./objectNodeTypes.js"; /** * Contains state and an internal API for managing {@link TreeNode}s. @@ -34,7 +38,17 @@ export class TreeNodeKernel implements Listenable { const offChildrenChanged = anchorNode.on( "childrenChangedAfterBatch", ({ changedFields }) => { - this.#events.emit("nodeChanged", { changedProperties: changedFields }); + const nodeSchema = getSimpleNodeSchema(getFlexNode(this.node).schema); + const changedProperties = isObjectNodeSchema(nodeSchema) + ? new Set( + [...changedFields].map( + (field) => + nodeSchema.storedKeyToViewKeyMap.get(field) ?? + fail(`Could not find stored key '${field}' in schema.`), + ), + ) + : new Set(changedFields); + this.#events.emit("nodeChanged", { changedProperties }); }, ); diff --git a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 3822868e116e..4e8e29ecaae7 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -874,5 +874,41 @@ describe("treeNodeApi", () => { assert.deepEqual([...root], [3, 2]); assert.equal(listenerFired, true, "'nodeChanged' should have fired"); }); + + it(`'nodeChanged' uses view keys, not stored keys, for the list of changed properties`, () => { + const sb = new SchemaFactory("test"); + class treeSchema extends sb.object("root", { + prop1: sb.optional(sb.number, { key: "stored-prop1" }), + prop2: sb.optional(sb.number, { key: "stored-prop2" }), + prop3: sb.optional(sb.number, { key: "stored-prop3" }), + }) {} + + const view = getView(new TreeViewConfiguration({ schema: treeSchema })); + view.initialize({ prop1: 1, prop2: 2 /* , prop3: 0 */ }); + const root = view.root; + + let listenerFired = false; + Tree.on(root, "nodeChanged", ({ changedProperties }) => { + // This is the main validation for the test + assert.deepEqual(changedProperties, new Set(["prop1", "prop2", "prop3"])); + listenerFired = true; + }); + + 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 then, 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); + + // Validate changes actually took place and all listeners fired + assert.equal(root.prop1, 2, "'prop1' value did not change as expected"); + assert.equal(root.prop2, undefined, "'prop2' value did not change as expected"); + assert.equal(root.prop3, 3, "'prop3' value did not change as expected"); + assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + }); }); }); From 604c2895298ed99e3d9c62ce1b56586e0b510d58 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Tue, 30 Jul 2024 23:10:34 +0000 Subject: [PATCH 14/34] Fix circular dependency --- packages/dds/tree/src/simple-tree/treeNodeKernel.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index 645e37722ca2..0ae0c4c507fc 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -14,7 +14,6 @@ import { TreeStatus, treeStatusFromAnchorCache, } from "../feature-libraries/index.js"; -import { getFlexNode } from "./index.js"; import { getSimpleNodeSchema } from "./schemaCaching.js"; import { fail } from "../util/index.js"; import { isObjectNodeSchema } from "./objectNodeTypes.js"; @@ -38,7 +37,9 @@ export class TreeNodeKernel implements Listenable { const offChildrenChanged = anchorNode.on( "childrenChangedAfterBatch", ({ changedFields }) => { - const nodeSchema = getSimpleNodeSchema(getFlexNode(this.node).schema); + const flexNode = anchorNode.slots.get(flexTreeSlot); + assert(flexNode !== undefined, "Flex node does not exist"); + const nodeSchema = getSimpleNodeSchema(flexNode.schema); const changedProperties = isObjectNodeSchema(nodeSchema) ? new Set( [...changedFields].map( From 0b0e2ad091c4be5fa94cb0e6469af79808be21f6 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Tue, 30 Jul 2024 23:28:27 +0000 Subject: [PATCH 15/34] Changeset --- .changeset/wet-hoops-drop.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 .changeset/wet-hoops-drop.md diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md new file mode 100644 index 000000000000..23e2704efa01 --- /dev/null +++ b/.changeset/wet-hoops-drop.md @@ -0,0 +1,17 @@ +--- +"fluid-framework": minor +"@fluidframework/tree": minor +--- + +nodeChanged event now includes the list of properties that changed + +The payload of the `nodeChanged` event emitted by SharedTree now 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 "view 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 list of properties will always be empty. From 25b528a9d607a3bf4a96ef3100619cafd19fdec8 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 31 Jul 2024 20:13:07 +0000 Subject: [PATCH 16/34] PR feedback --- .../dds/tree/api-report/tree.alpha.api.md | 4 +- packages/dds/tree/api-report/tree.beta.api.md | 4 +- .../dds/tree/api-report/tree.public.api.md | 4 +- .../tree/src/simple-tree/treeNodeKernel.ts | 5 +- packages/dds/tree/src/simple-tree/types.ts | 4 +- .../tree/src/test/simple-tree/treeApi.spec.ts | 101 ++++++------------ 6 files changed, 47 insertions(+), 75 deletions(-) diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 25b1f203516d..7eff0d2401aa 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -390,8 +390,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 097d73ce5a3d..9c13ce499923 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -390,8 +390,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index f602ffdfeb06..962f7ce95164 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -390,8 +390,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index 0ae0c4c507fc..7cf80f15e580 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -42,13 +42,14 @@ export class TreeNodeKernel implements Listenable { const nodeSchema = getSimpleNodeSchema(flexNode.schema); const changedProperties = isObjectNodeSchema(nodeSchema) ? new Set( - [...changedFields].map( + Array.from( + changedFields, (field) => nodeSchema.storedKeyToViewKeyMap.get(field) ?? fail(`Could not find stored key '${field}' in schema.`), ), ) - : new Set(changedFields); + : changedFields; this.#events.emit("nodeChanged", { changedProperties }); }, ); diff --git a/packages/dds/tree/src/simple-tree/types.ts b/packages/dds/tree/src/simple-tree/types.ts index 3bf0e4883673..497b1e0303e8 100644 --- a/packages/dds/tree/src/simple-tree/types.ts +++ b/packages/dds/tree/src/simple-tree/types.ts @@ -92,7 +92,9 @@ export interface TreeChangeEvents { * 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). */ - nodeChanged({ changedProperties }: { changedProperties: ReadonlySet }): void; + nodeChanged({ + changedProperties, + }: { readonly changedProperties: ReadonlySet }): 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/test/simple-tree/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts index 4e8e29ecaae7..92e1d373b95a 100644 --- a/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/treeApi.spec.ts @@ -771,45 +771,39 @@ describe("treeNodeApi", () => { it(`'nodeChanged' includes the names of changed properties (objectNode)`, () => { const sb = new SchemaFactory("test"); - class treeSchema extends sb.object("root", { + 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: treeSchema })); - view.initialize({ prop1: 1, prop2: 2 /* , prop3: 0 */ }); + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize({ prop1: 1, prop2: 2 }); const root = view.root; - let listenerFired = false; - Tree.on(root, "nodeChanged", ({ changedProperties }) => { - // This is the main validation for the test - assert.deepEqual(changedProperties, new Set(["prop1", "prop2", "prop3"])); - listenerFired = true; - }); + const eventLog: ReadonlySet[] = []; + Tree.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 then, try to cover all of them. + // 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); - // Validate changes actually took place and all listeners fired - assert.equal(root.prop1, 2, "'prop1' value did not change as expected"); - assert.equal(root.prop2, undefined, "'prop2' value did not change as expected"); - assert.equal(root.prop3, 3, "'prop3' value did not change as expected"); - assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + assert.deepEqual(eventLog, [new Set(["prop1", "prop2", "prop3"])]); }); it(`'nodeChanged' includes the names of changed properties (mapNode)`, () => { const sb = new SchemaFactory("test"); - class treeSchema extends sb.map("root", [sb.number]) {} + class TestNode extends sb.map("root", [sb.number]) {} - const view = getView(new TreeViewConfiguration({ schema: treeSchema })); + const view = getView(new TreeViewConfiguration({ schema: TestNode })); view.initialize( new Map([ ["key1", 1], @@ -818,97 +812,72 @@ describe("treeNodeApi", () => { ); const root = view.root; - let listenerFired = false; - Tree.on(root, "nodeChanged", ({ changedProperties }) => { - // This is the main validation for the test - assert.deepEqual(changedProperties, new Set(["key1", "key2", "key3"])); - listenerFired = true; - }); + const eventLog: ReadonlySet[] = []; + Tree.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 then, try to cover all of them. + // 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); - // Validate changes actually took place and all listeners fired - assert.equal(root.get("key1"), 0, "'key1' value did not change as expected"); - assert.equal(root.get("key2"), undefined, "'key2' value did not change as expected"); - assert.equal(root.get("key3"), 3, "'key3' value did not change as expected"); - assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + assert.deepEqual(eventLog, [new Set(["key1", "key2", "key3"])]); }); - it(`'nodeChanged' includes the names of changed properties (arrayNode)`, () => { + it(`'nodeChanged' does not include the names of changed properties (arrayNode)`, () => { const sb = new SchemaFactory("test"); - class treeSchema extends sb.array("root", [sb.number]) {} + class TestNode extends sb.array("root", [sb.number]) {} - const view = getView(new TreeViewConfiguration({ schema: treeSchema })); + const view = getView(new TreeViewConfiguration({ schema: TestNode })); view.initialize([1, 2]); const root = view.root; - let listenerFired = false; - Tree.on(root, "nodeChanged", ({ changedProperties }) => { - // This is the main validation for the test - assert.deepEqual(changedProperties, new Set()); - listenerFired = true; - }); + const eventLog: ReadonlySet[] = []; + Tree.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 then, try to cover all of them. + // But since we know them, try to cover all of them. forkView.root.insertAtEnd(3); // Append to array forkView.root.removeAt(0); // Remove from arrray forkView.root.moveRangeToEnd(0, 1); // Move within array view.checkout.merge(forkCheckout); - // Validate changes actually took place and all listeners fired - // Note: as of 2024-07-29, if the schema for the root is defined with a name - // (`class treeSchema extends sb.array("root", [sb.number])` where "root" is the name), we need to use Array.from() - // or array spread in order to compare the node to another array. - assert.deepEqual([...root], [3, 2]); - assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + assert.deepEqual(eventLog, [new Set()]); }); it(`'nodeChanged' uses view keys, not stored keys, for the list of changed properties`, () => { const sb = new SchemaFactory("test"); - class treeSchema extends sb.object("root", { + class TestNode extends sb.object("root", { prop1: sb.optional(sb.number, { key: "stored-prop1" }), - prop2: sb.optional(sb.number, { key: "stored-prop2" }), - prop3: sb.optional(sb.number, { key: "stored-prop3" }), }) {} - const view = getView(new TreeViewConfiguration({ schema: treeSchema })); - view.initialize({ prop1: 1, prop2: 2 /* , prop3: 0 */ }); + const view = getView(new TreeViewConfiguration({ schema: TestNode })); + view.initialize({ prop1: 1 }); const root = view.root; - let listenerFired = false; - Tree.on(root, "nodeChanged", ({ changedProperties }) => { - // This is the main validation for the test - assert.deepEqual(changedProperties, new Set(["prop1", "prop2", "prop3"])); - listenerFired = true; - }); + const eventLog: ReadonlySet[] = []; + Tree.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 then, try to cover all of them. - forkView.root.prop1 = 2; // Replace - forkView.root.prop2 = undefined; // Detach - forkView.root.prop3 = 3; // Attach + forkView.root.prop1 = 2; view.checkout.merge(forkCheckout); - // Validate changes actually took place and all listeners fired - assert.equal(root.prop1, 2, "'prop1' value did not change as expected"); - assert.equal(root.prop2, undefined, "'prop2' value did not change as expected"); - assert.equal(root.prop3, 3, "'prop3' value did not change as expected"); - assert.equal(listenerFired, true, "'nodeChanged' should have fired"); + assert.deepEqual(eventLog, [new Set(["prop1"])]); }); }); }); From f5553b6d9df7f78236a9b3175097becd486f92bb Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 31 Jul 2024 20:30:14 +0000 Subject: [PATCH 17/34] Move cherk for array case to treeNodeKernel --- packages/dds/tree/src/core/tree/anchorSet.ts | 7 +--- .../tree/src/simple-tree/treeNodeKernel.ts | 33 ++++++++++++------- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index a88e99bb0be7..35f635c6c40a 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -778,12 +778,7 @@ export class AnchorSet implements Listenable, AnchorLocator const fieldKeys = this.bufferedEvents .filter((e) => e.node === node && e.event === event) .map((e) => e.changedField as FieldKey); - // For array nodes, the field key will always be an empty string. - // In that case we want to emit the event with an empty set, instead of a set with the empty string as only member. - const changedFields = fieldKeys.every((x) => x === "") - ? new Set() - : new Set(fieldKeys); - node.events.emit(event, { anchor: node, changedFields }); + node.events.emit(event, { anchor: node, changedFields: new Set(fieldKeys) }); } else { node.events.emit(event, node); } diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index 7cf80f15e580..4a5510d9aa3c 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -6,7 +6,7 @@ import { assert } from "@fluidframework/core-utils/internal"; import { createEmitter, type Listenable, type Off } from "../events/index.js"; import type { TreeChangeEvents, TreeNode } from "./types.js"; -import type { AnchorNode } from "../core/index.js"; +import type { AnchorNode, FieldKey } from "../core/index.js"; import { flexTreeSlot, isFreedSymbol, @@ -17,6 +17,7 @@ import { import { getSimpleNodeSchema } from "./schemaCaching.js"; import { fail } from "../util/index.js"; import { isObjectNodeSchema } from "./objectNodeTypes.js"; +import { NodeKind } from "./schemaTypes.js"; /** * Contains state and an internal API for managing {@link TreeNode}s. @@ -40,16 +41,26 @@ export class TreeNodeKernel implements Listenable { const flexNode = anchorNode.slots.get(flexTreeSlot); assert(flexNode !== undefined, "Flex node does not exist"); const nodeSchema = getSimpleNodeSchema(flexNode.schema); - const changedProperties = isObjectNodeSchema(nodeSchema) - ? new Set( - Array.from( - changedFields, - (field) => - nodeSchema.storedKeyToViewKeyMap.get(field) ?? - fail(`Could not find stored key '${field}' in schema.`), - ), - ) - : changedFields; + let changedProperties: ReadonlySet; + if (isObjectNodeSchema(nodeSchema)) { + changedProperties = new Set( + Array.from( + changedFields, + (field) => + nodeSchema.storedKeyToViewKeyMap.get(field) ?? + fail(`Could not find stored key '${field}' in schema.`), + ), + ); + } else if (nodeSchema.kind === NodeKind.Array) { + // For array nodes, for now we don't have a good story of what we should expose as changed properties (indices? + // even if that means including all indices if something is added/removed at the beginning of the array?), so + // for now we just provide an empty set. In particular, we don't want to say "the key changed" + // which is what would happen if we just used the changedFields as the changedProperties because of the way + // array nodes work. + changedProperties = new Set(); + } else { + changedProperties = changedFields; + } this.#events.emit("nodeChanged", { changedProperties }); }, ); From 94df816664aec4feca3f19ae900595d26d829d60 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 31 Jul 2024 21:07:13 +0000 Subject: [PATCH 18/34] Remove unused import --- packages/dds/tree/src/simple-tree/treeNodeKernel.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts index 4a5510d9aa3c..fe166fee05f6 100644 --- a/packages/dds/tree/src/simple-tree/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/treeNodeKernel.ts @@ -6,7 +6,7 @@ import { assert } from "@fluidframework/core-utils/internal"; import { createEmitter, type Listenable, type Off } from "../events/index.js"; import type { TreeChangeEvents, TreeNode } from "./types.js"; -import type { AnchorNode, FieldKey } from "../core/index.js"; +import type { AnchorNode } from "../core/index.js"; import { flexTreeSlot, isFreedSymbol, From 000db718187ab4a4924f15f23f300d1c473d4df4 Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 31 Jul 2024 21:26:37 +0000 Subject: [PATCH 19/34] Move assert to better place --- packages/dds/tree/src/core/tree/anchorSet.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index 35f635c6c40a..6a4b24ce9320 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -790,14 +790,15 @@ export class AnchorSet implements Listenable, AnchorLocator () => this.anchorSet.events.emit("childrenChanging", this.anchorSet), ); }, - notifyChildrenChanged(field: FieldKey): void { + notifyChildrenChanged(): void { this.maybeWithNode( (p) => { + assert(this.parentField !== undefined, "Must be in a field"); p.events.emit("childrenChanged", p); this.bufferedEvents.push({ node: p, event: "childrenChangedAfterBatch", - changedField: field, + changedField: this.parentField, }); }, () => {}, @@ -847,8 +848,7 @@ export class AnchorSet implements Listenable, AnchorLocator attach(source: FieldKey, count: number, destination: PlaceIndex): void { this.notifyChildrenChanging(); this.attachEdit(source, count, destination); - assert(this.parentField !== undefined, "Must be in a field in order to attach"); - this.notifyChildrenChanged(this.parentField); + this.notifyChildrenChanged(); }, attachEdit(source: FieldKey, count: number, destination: PlaceIndex): void { assert( @@ -912,8 +912,7 @@ export class AnchorSet implements Listenable, AnchorLocator detach(source: Range, destination: FieldKey): void { this.notifyChildrenChanging(); this.detachEdit(source, destination); - assert(this.parentField !== undefined, "Must be in a field in order to attach"); - this.notifyChildrenChanged(this.parentField); + this.notifyChildrenChanged(); }, detachEdit(source: Range, destination: FieldKey): void { assert( @@ -999,8 +998,7 @@ export class AnchorSet implements Listenable, AnchorLocator this.notifyChildrenChanging(); this.detachEdit(range, oldContentDestination); this.attachEdit(newContentSource, range.end - range.start, range.start); - assert(this.parentField !== undefined, "Must be in a field in order to attach"); - this.notifyChildrenChanged(this.parentField); + this.notifyChildrenChanged(); }, destroy(detachedField: FieldKey, count: number): void { this.anchorSet.removeChildren( From 135659dd8df9f5c59af8b9016a20ac3ac047bd0e Mon Sep 17 00:00:00 2001 From: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Date: Wed, 31 Jul 2024 23:17:23 +0000 Subject: [PATCH 20/34] Update fluid-framework API reports --- .../fluid-framework/api-report/fluid-framework.beta.api.md | 4 ++-- .../api-report/fluid-framework.legacy.alpha.api.md | 4 ++-- .../api-report/fluid-framework.legacy.public.api.md | 4 ++-- .../fluid-framework/api-report/fluid-framework.public.api.md | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) 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 067fa2b38fa1..f184dfc6c43e 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 @@ -758,8 +758,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } 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 2c5980e4d65f..91fc4bb3eef8 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 @@ -1155,8 +1155,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } 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 7052843b6d09..d9243b75affc 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 @@ -798,8 +798,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } 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 b1daaf40e321..21d6fc066f72 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 @@ -758,8 +758,8 @@ export interface TreeArrayNodeUnsafe; + nodeChanged({ changedProperties, }: { + readonly changedProperties: ReadonlySet; }): void; treeChanged(): void; } From 760d8a3e72e701c1d9a64973f0dc03f73df1bd4f Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Thu, 15 Aug 2024 22:34:05 -0700 Subject: [PATCH 21/34] Avoid exporting SchematizingSimpleTreeView from simple-tree just to use in simple tree test utils. --- packages/dds/tree/src/shared-tree/index.ts | 2 -- packages/dds/tree/src/test/simple-tree/utils.ts | 7 +++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/dds/tree/src/shared-tree/index.ts b/packages/dds/tree/src/shared-tree/index.ts index cc1f45d56139..898c515b1c1b 100644 --- a/packages/dds/tree/src/shared-tree/index.ts +++ b/packages/dds/tree/src/shared-tree/index.ts @@ -33,8 +33,6 @@ export { buildTreeConfiguration, } from "./schematizeTree.js"; -export { SchematizingSimpleTreeView } from "./schematizingTreeView.js"; - export { type FlexTreeView, CheckoutFlexTreeView, diff --git a/packages/dds/tree/src/test/simple-tree/utils.ts b/packages/dds/tree/src/test/simple-tree/utils.ts index 62633a706cac..aa5367420d8d 100644 --- a/packages/dds/tree/src/test/simple-tree/utils.ts +++ b/packages/dds/tree/src/test/simple-tree/utils.ts @@ -29,10 +29,9 @@ import { // eslint-disable-next-line import/no-internal-modules import { toFlexSchema } from "../../simple-tree/toFlexSchema.js"; import { flexTreeFromForest, testIdCompressor, testRevisionTagCodec } from "../utils.js"; -import { - type ITreeCheckoutFork, - SchematizingSimpleTreeView, -} from "../../shared-tree/index.js"; +import type { ITreeCheckoutFork } from "../../shared-tree/index.js"; +// eslint-disable-next-line import/no-internal-modules +import { SchematizingSimpleTreeView } from "../../shared-tree/schematizingTreeView.js"; /** * Initializes a node with the given schema and content. From 0016bfaa447c25a0234fc3f2f715e0af4de85f62 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 19 Aug 2024 15:37:24 -0700 Subject: [PATCH 22/34] specialize for object and map nodes --- .../dds/tree/api-report/tree.alpha.api.md | 13 +++--- packages/dds/tree/api-report/tree.beta.api.md | 13 +++--- .../dds/tree/api-report/tree.public.api.md | 13 +++--- packages/dds/tree/src/index.ts | 1 + .../tree/src/simple-tree/api/treeNodeApi.ts | 6 +-- .../dds/tree/src/simple-tree/core/index.ts | 1 + .../dds/tree/src/simple-tree/core/types.ts | 41 ++++++++++++++----- packages/dds/tree/src/simple-tree/index.ts | 1 + 8 files changed, 60 insertions(+), 29 deletions(-) diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 9d00d81cde75..d8f1194db317 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; +// @public +export interface NodeChangedData { + readonly changedProperties: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -470,10 +475,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; treeChanged(): void; } @@ -516,7 +519,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 701e6219b91f..46b9bf827ab4 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; +// @public +export interface NodeChangedData { + readonly changedProperties: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -402,10 +407,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; treeChanged(): void; } @@ -448,7 +451,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index 914dde602e8d..fe24d49e5cde 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -218,6 +218,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @public +export interface NodeChangedData { + readonly changedProperties: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -402,10 +407,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; treeChanged(): void; } @@ -448,7 +451,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/src/index.ts b/packages/dds/tree/src/index.ts index 074ad528925e..0baf19254a22 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" diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index cb72a77d7480..846140a619fb 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -98,10 +98,10 @@ export interface TreeNodeApi { * @returns A callback function which will deregister the event. * This callback should be called only once. */ - on( - node: TreeNode, + on, TNode extends TreeNode>( + node: TNode, eventName: K, - listener: TreeChangeEvents[K], + listener: TreeChangeEvents[K], ): () => void; /** diff --git a/packages/dds/tree/src/simple-tree/core/index.ts b/packages/dds/tree/src/simple-tree/core/index.ts index 16fcbc12e406..952e8165fb2c 100644 --- a/packages/dds/tree/src/simple-tree/core/index.ts +++ b/packages/dds/tree/src/simple-tree/core/index.ts @@ -17,6 +17,7 @@ export { inPrototypeChain, type InternalTreeNode, privateToken, + type NodeChangedData, } from "./types.js"; export { type TreeNodeSchema, diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index 035285a9db18..000b181fafe6 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -24,6 +24,19 @@ import { isFlexTreeNode, type FlexTreeNode } from "../../feature-libraries/index */ export type Unhydrated = T; +/** + * Data included for {@link TreeChangeEvents.nodeChanged}. + * @public + */ +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}). + */ + readonly changedProperties: ReadonlySet; +} + /** * A collection of events that can be emitted by a {@link TreeNode}. * @@ -45,17 +58,15 @@ export type Unhydrated = T; * * @sealed @public */ -export interface TreeChangeEvents { +export interface TreeChangeEvents { /** - * Emitted by a node after a batch of changes has been applied to the tree, if a change affected the node, where a - * change is: + * Emitted by a node after a batch of changes has been applied to the tree, if any of the changes affected the node. * - * - For an object node, when the value of one of its properties changes (i.e., the property's value is set - * to something else, including `undefined`). + * - Object nodes define a change as the value of one of its properties changing (i.e., the property's value is set, including when set to undefined). * - * - For an array node, when an element is added, removed, or moved. + * - Array node define a change as when an element is added, removed, moved or replaced. * - * - For a map node, when an entry is added, updated, or removed. + * - Map nodes define a change as when an entry is added, updated, or removed. * * @remarks * This event is not emitted when: @@ -70,17 +81,23 @@ export interface TreeChangeEvents { * 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 it is emitted, the tree is guaranteed to be in-schema. + * 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: defined and document event ordering (ex: bottom up, with nodeChanged before treeCHange on each level). */ - nodeChanged({ - changedProperties, - }: { readonly changedProperties: ReadonlySet }): void; + nodeChanged( + data: NodeChangedData & + // For object and Map nodes, make properties specific to them required instead of optional: + TNode extends WithType + ? Required> + : unknown, + ): void; /** * Emitted by a node after a batch of changes has been applied to the tree, when something changed anywhere in the @@ -101,6 +118,8 @@ export interface TreeChangeEvents { treeChanged(): void; } +export type IsListener2 = TListener extends (...args: any[]) => void ? true : false; + /** * A non-{@link NodeKind.Leaf|leaf} SharedTree node. Includes objects, arrays, and maps. * diff --git a/packages/dds/tree/src/simple-tree/index.ts b/packages/dds/tree/src/simple-tree/index.ts index 37f484700ad3..eb07d6bca08c 100644 --- a/packages/dds/tree/src/simple-tree/index.ts +++ b/packages/dds/tree/src/simple-tree/index.ts @@ -19,6 +19,7 @@ export { type Unhydrated, type InternalTreeNode, isTreeNode, + type NodeChangedData, } from "./core/index.js"; export { type ITree, From c19fae1df6ef10aafeb3e7d5096cc14480019855 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 19 Aug 2024 16:08:46 -0700 Subject: [PATCH 23/34] Move event implementation, fix cycle. --- .../tree/src/simple-tree/api/treeNodeApi.ts | 43 +++++++- .../dds/tree/src/simple-tree/arrayNode.ts | 15 +-- .../src/simple-tree/core/treeNodeKernel.ts | 97 ++++++------------- 3 files changed, 72 insertions(+), 83 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index 846140a619fb..d12671681477 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. @@ -167,7 +168,45 @@ export const treeNodeApi: TreeNodeApi = { eventName: K, listener: TreeChangeEvents[K], ): Off { - return getKernel(node).on(eventName, listener); + const kernel = getKernel(node); + if (kernel.hydrated === undefined) { + throw new UsageError( + "Subscribing to events for a node which is Unhydrated is unsupported", + ); + } + const anchorNode = kernel.hydrated.anchorNode; + + switch (eventName) { + case "nodeChanged": { + const nodeSchema = kernel.schema; + if (isObjectNodeSchema(nodeSchema)) { + return anchorNode.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({ changedFields: changedProperties }); + }); + } else if (nodeSchema.kind === NodeKind.Array) { + return anchorNode.on("childrenChangedAfterBatch", () => { + listener({}); + }); + } else { + return anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { + listener({ changedFields }); + }); + } + } + case "treeChanged": { + anchorNode.on("subtreeChangedAfterBatch", listener); + } + default: + throw new UsageError(`No event named ${JSON.stringify(eventName)}.`); + } }, status(node: TreeNode): TreeStatus { return getKernel(node).getStatus(); @@ -245,7 +284,7 @@ export function tryGetSchema(value: unknown): undefined | TreeNodeSchema { return booleanSchema; case "object": { if (isTreeNode(value)) { - // This case could be optimized, for example by placing the simple schema in a symbol on tree nodes. + // TODO: This case could be optimized, for example by placing the simple schema in a symbol on tree nodes. return tryGetTreeNodeSchema(value); } if (value === null) { diff --git a/packages/dds/tree/src/simple-tree/arrayNode.ts b/packages/dds/tree/src/simple-tree/arrayNode.ts index 62eb8c00dfcc..5c2432771048 100644 --- a/packages/dds/tree/src/simple-tree/arrayNode.ts +++ b/packages/dds/tree/src/simple-tree/arrayNode.ts @@ -670,19 +670,10 @@ abstract class CustomArrayNodeBase protected abstract get simpleSchema(): T; - /** - * Generation number which is incremented any time we have an edit on the node. - * Used during iteration to make sure there has been no edits that were concurrently made. - */ - #generationNumber: number = 0; - public constructor( input: Iterable> | InternalTreeNode, ) { super(input); - getKernel(this).on("nodeChanged", () => { - this.#generationNumber += 1; - }); } #mapTreesFromFieldData(value: Insertable): ExclusiveMapTree[] { @@ -884,17 +875,17 @@ abstract class CustomArrayNodeBase } public values(): IterableIterator> { - return this.generateValues(this.#generationNumber); + return this.generateValues(getKernel(this).generationNumber); } private *generateValues( initialLastUpdatedStamp: number, ): Generator> { - if (initialLastUpdatedStamp !== this.#generationNumber) { + if (initialLastUpdatedStamp !== getKernel(this).generationNumber) { throw new UsageError(`Concurrent editing and iteration is not allowed.`); } for (let i = 0; i < this.length; i++) { yield this.at(i) ?? fail("Index is out of bounds"); - if (initialLastUpdatedStamp !== this.#generationNumber) { + if (initialLastUpdatedStamp !== getKernel(this).generationNumber) { throw new UsageError(`Concurrent editing and iteration is not allowed.`); } } diff --git a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index 36d9f1255a6c..e72488e1ec69 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -4,8 +4,8 @@ */ import { assert } from "@fluidframework/core-utils/internal"; -import { createEmitter, type Listenable, type Off } from "../../events/index.js"; -import type { TreeChangeEvents, TreeNode, Unhydrated } from "./types.js"; +import type { Off } from "../../events/index.js"; +import type { TreeNode, Unhydrated } from "./types.js"; import type { AnchorNode } from "../../core/index.js"; import { flexTreeSlot, @@ -14,10 +14,7 @@ import { TreeStatus, treeStatusFromAnchorCache, } from "../../feature-libraries/index.js"; -import { getSimpleNodeSchema } from "./schemaCaching.js"; -import { fail } from "../../util/index.js"; -import { isObjectNodeSchema } from "../objectNodeTypes.js"; // TODO: layer violation: simple-tree/core should not depend on specific node-kinds. Node kind specific logic should live in the files implementing those node kinds, or in simple-tree/api. -import { NodeKind, type TreeNodeSchema } from "./treeNodeSchema.js"; +import type { TreeNodeSchema } from "./treeNodeSchema.js"; const treeNodeToKernel = new WeakMap(); @@ -63,12 +60,17 @@ export function tryGetTreeNodeSchema(value: unknown): undefined | TreeNodeSchema * The kernel has the same lifetime as the node and spans both its unhydrated and hydrated states. * When hydration occurs, the kernel is notified via the {@link TreeNodeKernel.hydrate | hydrate} method. */ -export class TreeNodeKernel implements Listenable { - #hydrated?: { +export class TreeNodeKernel { + /** + * Generation number which is incremented any time we have an edit on the node. + * Used during iteration to make sure there has been no edits that were concurrently made. + */ + public generationNumber: number = 0; + + public hydrated?: { anchorNode: AnchorNode; - offAnchorNode: Off; + offAnchorNode: Set; }; - #events = createEmitter(); /** * Create a TreeNodeKernel which can be looked up with {@link getKernel}. @@ -84,72 +86,36 @@ export class TreeNodeKernel implements Listenable { } public hydrate(anchorNode: AnchorNode): void { - // TODO: this logic being in hydrate is problematic for two reasons: - // 1: It depends on node kinds, which are not supposed to be used in the folder. - // 2. It won't work for nodes which are unhydrated, which are getting editing support and should have these events. - // This logic should probably move to TreeNode API where the events are actually exposed to users. - const offChildrenChanged = anchorNode.on( - "childrenChangedAfterBatch", - ({ changedFields }) => { - const flexNode = anchorNode.slots.get(flexTreeSlot); - assert(flexNode !== undefined, "Flex node does not exist"); - const nodeSchema = getSimpleNodeSchema(flexNode.schema); - let changedProperties: ReadonlySet; - if (isObjectNodeSchema(nodeSchema)) { - changedProperties = new Set( - Array.from( - changedFields, - (field) => - nodeSchema.storedKeyToPropertyKey.get(field) ?? - fail(`Could not find stored key '${field}' in schema.`), - ), - ); - } else if (nodeSchema.kind === NodeKind.Array) { - // For array nodes, for now we don't have a good story of what we should expose as changed properties (indices? - // even if that means including all indices if something is added/removed at the beginning of the array?), so - // for now we just provide an empty set. In particular, we don't want to say "the key changed" - // which is what would happen if we just used the changedFields as the changedProperties because of the way - // array nodes work. - changedProperties = new Set(); - } else { - changedProperties = changedFields; - } - this.#events.emit("nodeChanged", { changedProperties }); - }, - ); - - const offSubtreeChanged = anchorNode.on("subtreeChangedAfterBatch", () => { - this.#events.emit("treeChanged"); - }); - - const offAfterDestroy = anchorNode.on("afterDestroy", () => this.dispose()); - - this.#hydrated = { + this.hydrated = { anchorNode, - offAnchorNode: () => { - offChildrenChanged(); - offSubtreeChanged(); - offAfterDestroy(); - }, + offAnchorNode: new Set([ + anchorNode.on("afterDestroy", () => this.dispose()), + // TODO: this should be triggered on change even for unhydrated nodes. + anchorNode.on("childrenChanging", () => { + this.generationNumber += 1; + }), + ]), }; } public dehydrate(): void { - this.#hydrated?.offAnchorNode?.(); - this.#hydrated = undefined; + for (const off of this.hydrated?.offAnchorNode ?? []) { + off(); + } + this.hydrated = undefined; } public isHydrated(): boolean { - return this.#hydrated !== undefined; + return this.hydrated !== undefined; } public getStatus(): TreeStatus { - if (this.#hydrated?.anchorNode === undefined) { + if (this.hydrated?.anchorNode === undefined) { return TreeStatus.New; } // TODO: Replace this check with the proper check against the cursor state when the cursor becomes part of the kernel - const flex = this.#hydrated.anchorNode.slots.get(flexTreeSlot); + const flex = this.hydrated.anchorNode.slots.get(flexTreeSlot); if (flex !== undefined) { assert(flex instanceof LazyEntity, 0x9b4 /* Unexpected flex node implementation */); if (flex[isFreedSymbol]()) { @@ -157,14 +123,7 @@ export class TreeNodeKernel implements Listenable { } } - return treeStatusFromAnchorCache(this.#hydrated.anchorNode); - } - - public on( - eventName: K, - listener: TreeChangeEvents[K], - ): Off { - return this.#events.on(eventName, listener); + return treeStatusFromAnchorCache(this.hydrated.anchorNode); } public dispose(): void { From 07b9c0b588a8567a836437f979ec1bc781e10976 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Mon, 19 Aug 2024 16:29:28 -0700 Subject: [PATCH 24/34] Fix cyclic deps and add node kind based typing --- packages/dds/tree/api-report/tree.alpha.api.md | 4 ++-- packages/dds/tree/api-report/tree.beta.api.md | 4 ++-- packages/dds/tree/api-report/tree.public.api.md | 4 ++-- .../dds/tree/src/simple-tree/api/treeNodeApi.ts | 9 ++++----- packages/dds/tree/src/simple-tree/core/types.ts | 8 ++++---- .../tree/src/test/simple-tree/api/treeApi.spec.ts | 10 ++++------ .../api-report/fluid-framework.beta.api.md | 13 ++++++++----- .../api-report/fluid-framework.legacy.alpha.api.md | 13 ++++++++----- .../api-report/fluid-framework.legacy.public.api.md | 13 ++++++++----- .../api-report/fluid-framework.public.api.md | 13 ++++++++----- 10 files changed, 50 insertions(+), 41 deletions(-) diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index d8f1194db317..f30811d78853 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -288,7 +288,7 @@ type NodeBuilderDataUnsafe> = T extends Tre // @public export interface NodeChangedData { - readonly changedProperties: ReadonlySet; + readonly changedProperties?: ReadonlySet; } // @public @@ -476,7 +476,7 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 46b9bf827ab4..397af8e10d8d 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -220,7 +220,7 @@ type NodeBuilderDataUnsafe> = T extends Tre // @public export interface NodeChangedData { - readonly changedProperties: ReadonlySet; + readonly changedProperties?: ReadonlySet; } // @public @@ -408,7 +408,7 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index fe24d49e5cde..0ee4443d8b1e 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -220,7 +220,7 @@ type NodeBuilderDataUnsafe> = T extends Tre // @public export interface NodeChangedData { - readonly changedProperties: ReadonlySet; + readonly changedProperties?: ReadonlySet; } // @public @@ -408,7 +408,7 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & TNode extends WithType ? Required> : unknown): void; + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index d12671681477..9a28aba85d07 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -175,7 +175,6 @@ export const treeNodeApi: TreeNodeApi = { ); } const anchorNode = kernel.hydrated.anchorNode; - switch (eventName) { case "nodeChanged": { const nodeSchema = kernel.schema; @@ -189,20 +188,20 @@ export const treeNodeApi: TreeNodeApi = { fail(`Could not find stored key '${field}' in schema.`), ), ); - listener({ changedFields: changedProperties }); + listener({ changedProperties }); }); } else if (nodeSchema.kind === NodeKind.Array) { return anchorNode.on("childrenChangedAfterBatch", () => { - listener({}); + listener({ changedProperties: undefined }); }); } else { return anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { - listener({ changedFields }); + listener({ changedProperties: changedFields }); }); } } case "treeChanged": { - anchorNode.on("subtreeChangedAfterBatch", listener); + return anchorNode.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 000b181fafe6..e81d6a71e722 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -34,7 +34,7 @@ export interface NodeChangedData { * @remarks * This only includes changes to the node itself (which would trigger {@link TreeChangeEvents.nodeChanged}). */ - readonly changedProperties: ReadonlySet; + readonly changedProperties?: ReadonlySet; } /** @@ -94,9 +94,9 @@ export interface TreeChangeEvents { nodeChanged( data: NodeChangedData & // For object and Map nodes, make properties specific to them required instead of optional: - TNode extends WithType - ? Required> - : unknown, + (TNode extends WithType + ? Required> + : unknown), ): void; /** 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 d367ba328890..49bf12a86667 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 @@ -838,22 +838,20 @@ describe("treeNodeApi", () => { view.initialize([1, 2]); const root = view.root; - const eventLog: ReadonlySet[] = []; - Tree.on(root, "nodeChanged", ({ changedProperties }) => - eventLog.push(changedProperties), - ); + const eventLog: (ReadonlySet | undefined)[] = []; + Tree.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 arrray + forkView.root.removeAt(0); // Remove from array forkView.root.moveRangeToEnd(0, 1); // Move within array view.checkout.merge(forkCheckout); - assert.deepEqual(eventLog, [new Set()]); + assert.deepEqual(eventLog, [undefined]); }); it(`'nodeChanged' uses view keys, not stored keys, for the list of changed properties`, () => { 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 9cdfbaad2391..20adc8f79a3d 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 @@ -563,6 +563,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @public +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -774,10 +779,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } @@ -820,7 +823,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 e12a23b48f36..0f0e2e9bdf01 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 @@ -869,6 +869,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @public +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -1171,10 +1176,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } @@ -1217,7 +1220,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 47b9261e3e8c..4c7afea67ab0 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 @@ -599,6 +599,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @public +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -814,10 +819,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } @@ -860,7 +863,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 90b325a974dc..83109ae05a1e 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 @@ -563,6 +563,11 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; +// @public +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet; +} + // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -774,10 +779,8 @@ export interface TreeArrayNodeUnsafe; - }): void; +export interface TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } @@ -820,7 +823,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; From 7f8ba44b11437f9bd6c17b302b48065debd862a7 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 20 Aug 2024 12:59:46 -0700 Subject: [PATCH 25/34] Fix unhydrated events --- packages/dds/tree/src/core/tree/anchorSet.ts | 7 +- packages/dds/tree/src/events/events.ts | 4 +- .../tree/src/simple-tree/api/treeNodeApi.ts | 14 +-- .../src/simple-tree/core/treeNodeKernel.ts | 97 ++++++++++++++++--- .../dds/tree/src/simple-tree/core/types.ts | 5 + .../test/simple-tree/unhydratedNode.spec.ts | 18 ++++ 6 files changed, 113 insertions(+), 32 deletions(-) diff --git a/packages/dds/tree/src/core/tree/anchorSet.ts b/packages/dds/tree/src/core/tree/anchorSet.ts index 41dacc394362..f4d88d181978 100644 --- a/packages/dds/tree/src/core/tree/anchorSet.ts +++ b/packages/dds/tree/src/core/tree/anchorSet.ts @@ -30,7 +30,7 @@ import type { RangeUpPath, UpPath, } from "./pathTree.js"; -import { EmptyKey, type Value } from "./types.js"; +import { EmptyKey } from "./types.js"; import type { DeltaVisitor } from "./visitDelta.js"; import type { PathVisitor } from "./visitPath.js"; import type { AnnouncedVisitor } from "./visitorUtils.js"; @@ -180,11 +180,6 @@ export interface AnchorEvents { * fire when something _may_ have changed or _may_ be about to change. */ subtreeChangedAfterBatch(anchor: AnchorNode): void; - - /** - * Value on this node is changing. - */ - valueChanging(anchor: AnchorNode, value: Value): void; } /** diff --git a/packages/dds/tree/src/events/events.ts b/packages/dds/tree/src/events/events.ts index 0303b9843c6b..a64e77041e36 100644 --- a/packages/dds/tree/src/events/events.ts +++ b/packages/dds/tree/src/events/events.ts @@ -227,7 +227,9 @@ export class EventEmitter> ): void { const listeners = this.listeners.get(eventName); if (listeners !== undefined) { - const argArray: unknown[] = args; // TODO: Current TS (4.5.5) cannot spread `args` into `listener()`, but future versions (e.g. 4.8.4) can. + // Current TS (5.4.5) cannot spread `args` into `listener()`, but assigning to an array first makes it work. + const argArray: unknown[] = args; + // This explicitly copies listeners so that new listeners added during this call to emit will not receive this event. for (const [off, listener] of [...listeners]) { // If listener has been unsubscribed while invoking other listeners, skip it. diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index 9a28aba85d07..d0db55a24e27 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -169,17 +169,11 @@ export const treeNodeApi: TreeNodeApi = { listener: TreeChangeEvents[K], ): Off { const kernel = getKernel(node); - if (kernel.hydrated === undefined) { - throw new UsageError( - "Subscribing to events for a node which is Unhydrated is unsupported", - ); - } - const anchorNode = kernel.hydrated.anchorNode; switch (eventName) { case "nodeChanged": { const nodeSchema = kernel.schema; if (isObjectNodeSchema(nodeSchema)) { - return anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { + return kernel.on("childrenChangedAfterBatch", ({ changedFields }) => { const changedProperties = new Set( Array.from( changedFields, @@ -191,17 +185,17 @@ export const treeNodeApi: TreeNodeApi = { listener({ changedProperties }); }); } else if (nodeSchema.kind === NodeKind.Array) { - return anchorNode.on("childrenChangedAfterBatch", () => { + return kernel.on("childrenChangedAfterBatch", () => { listener({ changedProperties: undefined }); }); } else { - return anchorNode.on("childrenChangedAfterBatch", ({ changedFields }) => { + return kernel.on("childrenChangedAfterBatch", ({ changedFields }) => { listener({ changedProperties: changedFields }); }); } } case "treeChanged": { - return anchorNode.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/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index e72488e1ec69..bdb94bbb3674 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -4,9 +4,15 @@ */ import { assert } from "@fluidframework/core-utils/internal"; -import type { Off } from "../../events/index.js"; +import { + createEmitter, + type HasListeners, + type IEmitter, + type Listenable, + type Off, +} from "../../events/index.js"; import type { TreeNode, Unhydrated } from "./types.js"; -import type { AnchorNode } from "../../core/index.js"; +import type { AnchorEvents, AnchorNode } from "../../core/index.js"; import { flexTreeSlot, isFreedSymbol, @@ -60,18 +66,49 @@ export function tryGetTreeNodeSchema(value: unknown): undefined | TreeNodeSchema * The kernel has the same lifetime as the node and spans both its unhydrated and hydrated states. * When hydration occurs, the kernel is notified via the {@link TreeNodeKernel.hydrate | hydrate} method. */ -export class TreeNodeKernel { +export class TreeNodeKernel implements Listenable { + private disposed = false; + /** * Generation number which is incremented any time we have an edit on the node. * Used during iteration to make sure there has been no edits that were concurrently made. */ public generationNumber: number = 0; - public hydrated?: { + #hydrated?: { anchorNode: AnchorNode; offAnchorNode: Set; }; + /** + * Events registered before hydration. + * @remarks + * + */ + #preHydrationEvents?: Listenable & + IEmitter & + HasListeners; + + /** + * Get the listener. + * @remarks + * If before hydration, allocates and uses `#preHydrationEvents`, otherwise the anchorNode. + * This design avoids allocating `#preHydrationEvents` if unneeded. + * + * This design also avoids extra forwarding overhead for events from anchorNode and also + * avoids registering for events that the are unneeded. + * This means optimizations like skipping processing data in subtrees where no subtreeChanged events are subscribed to would be able to work, + * since this code does not unconditionally subscribe to those events (like a design simply forwarding all events would). + */ + get #events(): Listenable { + if (this.#hydrated === undefined) { + this.#preHydrationEvents ??= createEmitter(); + return this.#preHydrationEvents; + } else { + return this.#hydrated.anchorNode; + } + } + /** * Create a TreeNodeKernel which can be looked up with {@link getKernel}. * @remarks @@ -85,8 +122,15 @@ export class TreeNodeKernel { treeNodeToKernel.set(node, this); } + /** + * Transition from {@link Unhydrated} to hydrated. + * @remarks + * Happens at most once for any given node. + */ public hydrate(anchorNode: AnchorNode): void { - this.hydrated = { + assert(!this.disposed, "cannot use a disposed node"); + + this.#hydrated = { anchorNode, offAnchorNode: new Set([ anchorNode.on("afterDestroy", () => this.dispose()), @@ -96,26 +140,38 @@ export class TreeNodeKernel { }), ]), }; - } - public dehydrate(): void { - for (const off of this.hydrated?.offAnchorNode ?? []) { - off(); + // If needed, register forwarding emitters for events from before hydration + if (this.#preHydrationEvents !== undefined) { + for (const eventName of kernelEvents) { + if (this.#preHydrationEvents.hasListeners(eventName)) { + this.#hydrated.offAnchorNode.add( + // Argument is forwarded between matching events, so the type should be correct. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + anchorNode.on(eventName, (arg: any) => + this.#preHydrationEvents?.emit(eventName, arg), + ), + ); + } + } } - this.hydrated = undefined; } public isHydrated(): boolean { - return this.hydrated !== undefined; + assert(!this.disposed, "cannot use a disposed node"); + return this.#hydrated !== undefined; } public getStatus(): TreeStatus { - if (this.hydrated?.anchorNode === undefined) { + if (this.disposed) { + return TreeStatus.Deleted; + } + if (this.#hydrated?.anchorNode === undefined) { return TreeStatus.New; } // TODO: Replace this check with the proper check against the cursor state when the cursor becomes part of the kernel - const flex = this.hydrated.anchorNode.slots.get(flexTreeSlot); + const flex = this.#hydrated.anchorNode.slots.get(flexTreeSlot); if (flex !== undefined) { assert(flex instanceof LazyEntity, 0x9b4 /* Unexpected flex node implementation */); if (flex[isFreedSymbol]()) { @@ -123,11 +179,22 @@ export class TreeNodeKernel { } } - return treeStatusFromAnchorCache(this.hydrated.anchorNode); + return treeStatusFromAnchorCache(this.#hydrated.anchorNode); + } + + public on(eventName: K, listener: KernelEvents[K]): Off { + return this.#events.on(eventName, listener); } public dispose(): void { - this.dehydrate(); + this.disposed = true; + for (const off of this.#hydrated?.offAnchorNode ?? []) { + off(); + } // TODO: go to the context and remove myself from withAnchors } } + +const kernelEvents = ["childrenChangedAfterBatch", "subtreeChangedAfterBatch"] as const; + +type KernelEvents = Pick; diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index e81d6a71e722..eb73f65e376c 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -40,6 +40,11 @@ export interface NodeChangedData { /** * A collection of events that can be emitted by a {@link TreeNode}. * + * @remarks + * Currently events can be subscribed to for {@link Unhydrated} nodes, however no events will be triggered for the nodes until after they are hydrated. + * This is considered a known issue, and should be fixed in future versions. + * Do not rely on the fact that editing unhydrated nodes does not trigger their events. + * * @privateRemarks * TODO: add a way to subscribe to a specific field (for nodeChanged and treeChanged). * Probably have object node and map node specific APIs for this. 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 3b21b6449249..1788a3424820 100644 --- a/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts @@ -177,6 +177,24 @@ describe("Unhydrated nodes", () => { assert.equal(Tree.status(object), TreeStatus.New); }); + it("preserve events after 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 documents, 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; From 57b228e80c8e57fe7b605ec261b7b1ea9185d3fb Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 20 Aug 2024 14:01:38 -0700 Subject: [PATCH 26/34] Update chanegset --- .changeset/wet-hoops-drop.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md index 23e2704efa01..4510abd18204 100644 --- a/.changeset/wet-hoops-drop.md +++ b/.changeset/wet-hoops-drop.md @@ -14,4 +14,4 @@ See the documentation for `FieldProps` for more details about the distinction be 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 list of properties will always be empty. +For array nodes, the set of properties will always be undefined: there is currently not an API to get details about changes to an array. From 0d19c01375039f100f01d6b040aa5ba0cc558909 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:20:58 -0700 Subject: [PATCH 27/34] Apply suggestions from code review Co-authored-by: Alex Villarreal <716334+alexvy86@users.noreply.github.com> Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com> --- .changeset/wet-hoops-drop.md | 2 +- packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts | 4 ++-- packages/dds/tree/src/simple-tree/core/types.ts | 6 +++--- .../dds/tree/src/test/simple-tree/unhydratedNode.spec.ts | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md index 4510abd18204..8b1aa0b6cddb 100644 --- a/.changeset/wet-hoops-drop.md +++ b/.changeset/wet-hoops-drop.md @@ -14,4 +14,4 @@ See the documentation for `FieldProps` for more details about the distinction be 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 not an API to get details about changes to an array. +For array nodes, the set of properties will always be undefined: there is currently no API to get details about changes to an array. diff --git a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index bdb94bbb3674..2a3b158e5b2b 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -96,7 +96,7 @@ export class TreeNodeKernel implements Listenable { * This design avoids allocating `#preHydrationEvents` if unneeded. * * This design also avoids extra forwarding overhead for events from anchorNode and also - * avoids registering for events that the are unneeded. + * avoids registering for events that are unneeded. * This means optimizations like skipping processing data in subtrees where no subtreeChanged events are subscribed to would be able to work, * since this code does not unconditionally subscribe to those events (like a design simply forwarding all events would). */ @@ -128,7 +128,7 @@ export class TreeNodeKernel implements Listenable { * Happens at most once for any given node. */ public hydrate(anchorNode: AnchorNode): void { - assert(!this.disposed, "cannot use a disposed node"); + assert(!this.disposed, "cannot hydrate a disposed node"); this.#hydrated = { anchorNode, diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index eb73f65e376c..d5eca11ffafe 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -67,9 +67,9 @@ export interface 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 the value of one of its properties changing (i.e., the property's value is set, including when set to undefined). + * - 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 node define a change as when an element is added, removed, moved or replaced. + * - 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. * @@ -94,7 +94,7 @@ export interface TreeChangeEvents { * 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: defined and document event ordering (ex: bottom up, with nodeChanged before treeCHange on each level). + * TODO: define and document event ordering (ex: bottom up, with nodeChanged before treeCHange on each level). */ nodeChanged( data: NodeChangedData & 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 1788a3424820..c96f06d23567 100644 --- a/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/unhydratedNode.spec.ts @@ -177,7 +177,7 @@ describe("Unhydrated nodes", () => { assert.equal(Tree.status(object), TreeStatus.New); }); - it("preserve events after hydration - minimal", () => { + it("preserve event subscriptions during hydration - minimal", () => { const log: unknown[] = []; const leafObject = new TestLeaf({ value: "value" }); @@ -191,7 +191,7 @@ describe("Unhydrated nodes", () => { hydrate(TestLeaf, leafObject); leafObject.value = "new value"; // Assert that the event fired - // TODO: Eventually the order of events should be documents, and an approach like this can test that they are ordered as documented. + // 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"]); }); From e85c7cc7340ac31c9d266ac4c83d03eb9b45a18c Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:47:43 -0700 Subject: [PATCH 28/34] Better docs --- .changeset/wet-hoops-drop.md | 2 +- packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts | 8 +++++++- packages/dds/tree/src/simple-tree/core/types.ts | 6 ++---- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md index 8b1aa0b6cddb..5b737be12d49 100644 --- a/.changeset/wet-hoops-drop.md +++ b/.changeset/wet-hoops-drop.md @@ -10,7 +10,7 @@ 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 "view keys" and "stored keys". +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. diff --git a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts index 2a3b158e5b2b..1465473a5c07 100644 --- a/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts +++ b/packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts @@ -72,6 +72,12 @@ export class TreeNodeKernel implements Listenable { /** * Generation number which is incremented any time we have an edit on the node. * Used during iteration to make sure there has been no edits that were concurrently made. + * @remarks + * This is updated monotonically by this class when edits are applied. + * TODO: update this when applying edits to unhydrated trees. + * + * If TypeScript supported making this immutable from outside the class without making it readonly from inside, that would be used here, + * but they only way to do that is add a separate public accessor and make it private, which was deemed not worth the boilerplate, runtime overhead and bundle size. */ public generationNumber: number = 0; @@ -83,7 +89,7 @@ export class TreeNodeKernel implements Listenable { /** * Events registered before hydration. * @remarks - * + * As an optimization these are allocated lazily as they are usually unused. */ #preHydrationEvents?: Listenable & IEmitter & diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index d5eca11ffafe..d53c88c879ca 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -63,7 +63,7 @@ export interface NodeChangedData { * * @sealed @public */ -export interface TreeChangeEvents { +export interface TreeChangeEvents { /** * Emitted by a node after a batch of changes has been applied to the tree, if any of the changes affected the node. * @@ -94,7 +94,7 @@ export interface TreeChangeEvents { * 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). + * TODO: define and document event ordering (ex: bottom up, with nodeChanged before treeChange on each level). */ nodeChanged( data: NodeChangedData & @@ -123,8 +123,6 @@ export interface TreeChangeEvents { treeChanged(): void; } -export type IsListener2 = TListener extends (...args: any[]) => void ? true : false; - /** * A non-{@link NodeKind.Leaf|leaf} SharedTree node. Includes objects, arrays, and maps. * From 1b26fd2b97de80e12553d4d3040e37c1f53d043f Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:53:33 -0700 Subject: [PATCH 29/34] More comment fixes --- packages/dds/tree/src/simple-tree/core/types.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index d53c88c879ca..02f1de99d100 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -26,13 +26,17 @@ export type Unhydrated = T; /** * Data included for {@link TreeChangeEvents.nodeChanged}. - * @public + * @sealed @public */ 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; } From 5fbb0c7e15c207a78b9a9001a90fbf6db8360e17 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 3 Sep 2024 11:19:06 -0700 Subject: [PATCH 30/34] Refer to property keys not view keys --- .../tree/src/simple-tree/api/treeNodeApi.ts | 26 ++++----- .../dds/tree/src/simple-tree/objectNode.ts | 56 ++++++++++--------- .../dds/tree/src/simple-tree/schemaTypes.ts | 8 +-- .../dds/tree/src/simple-tree/simpleSchema.ts | 2 +- .../dds/tree/src/simple-tree/toFlexSchema.ts | 6 +- .../simple-tree/api/schemaFactory.spec.ts | 6 +- .../src/test/simple-tree/api/treeApi.spec.ts | 2 +- 7 files changed, 54 insertions(+), 52 deletions(-) diff --git a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts index d0db55a24e27..c49cca30e0b7 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -156,8 +156,8 @@ export const treeNodeApi: TreeNodeApi = { } // The flex-domain strictly operates in terms of "stored keys". - // To find the associated developer-facing "view key", we need to look up the field associated with - // the stored key from the flex-domain, and get view key its simple-domain counterpart was created with. + // To find the associated developer-facing "property key", we need to look up the field associated with + // the stored key from the flex-domain, and get property key its simple-domain counterpart was created with. const storedKey = getStoredKey(node); const parentSchema = treeNodeApi.schema(parent); const viewKey = getViewKeyFromStoredKey(parentSchema, storedKey); @@ -297,7 +297,7 @@ export function tryGetSchema(value: unknown): undefined | TreeNodeSchema { */ function getStoredKey(node: TreeNode): string | number { // Note: the flex domain strictly works with "stored keys", and knows nothing about the developer-facing - // "view keys". + // "property keys". const parentField = getOrCreateInnerNode(node).parentField; if (parentField.parent.schema.kind.multiplicity === Multiplicity.Sequence) { // The parent of `node` is an array node @@ -309,14 +309,14 @@ function getStoredKey(node: TreeNode): string | number { } /** - * Given a node schema, gets the view key corresponding with the provided {@link FieldProps.key | stored key}. + * Given a node schema, gets the property key corresponding with the provided {@link FieldProps.key | stored key}. */ function getViewKeyFromStoredKey( schema: TreeNodeSchema, storedKey: string | number, ): string | number { - // Only object nodes have the concept of a "stored key", differentiated from the developer-facing "view key". - // For any other kind of node, the stored key and the view key are the same. + // Only object nodes have the concept of a "stored key", differentiated from the developer-facing "property key". + // For any other kind of node, the stored key and the property key are the same. if (schema.kind !== NodeKind.Object) { return storedKey; } @@ -324,18 +324,18 @@ function getViewKeyFromStoredKey( const fields = schema.info as Record; // Invariants: - // - The set of all view keys under an object must be unique. - // - The set of all stored keys (including those implicitly created from view keys) must be unique. - // To find the view key associated with the provided stored key, first check for any stored key matches (which are optionally populated). - // If we don't find any, then search for a matching view key. - for (const [viewKey, fieldSchema] of Object.entries(fields)) { + // - The set of all property keys under an object must be unique. + // - The set of all stored keys (including those implicitly created from property keys) must be unique. + // To find the property key associated with the provided stored key, first check for any stored key matches (which are optionally populated). + // If we don't find any, then search for a matching property key. + for (const [propertyKey, fieldSchema] of Object.entries(fields)) { if (fieldSchema instanceof FieldSchema && fieldSchema.props?.key === storedKey) { - return viewKey; + return propertyKey; } } if (fields[storedKey] === undefined) { - fail("Existing stored key should always map to a view key"); + fail("Existing stored key should always map to a property key"); } return storedKey; diff --git a/packages/dds/tree/src/simple-tree/objectNode.ts b/packages/dds/tree/src/simple-tree/objectNode.ts index 3d9896ee2086..7266bf10fc25 100644 --- a/packages/dds/tree/src/simple-tree/objectNode.ts +++ b/packages/dds/tree/src/simple-tree/objectNode.ts @@ -121,10 +121,10 @@ export type InsertableObjectFromSchemaRecord< >; /** - * Maps from simple field keys ("view" keys) to information about the field. + * Maps from simple field keys ("property" keys) to information about the field. * * @remarks - * A missing entry for a given view key indicates that no such field exists. + * A missing entry for a given property key indicates that no such field exists. * Keys with symbols are currently never used, but allowed to make lookups on non-field things * (returning undefined) easier. */ @@ -134,13 +134,13 @@ export type SimpleKeyMap = ReadonlyMap< >; /** - * Caches the mappings from view keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}. + * Caches the mappings from property keys to stored keys for the provided object field schemas in {@link simpleKeyToFlexKeyCache}. */ function createFlexKeyMapping(fields: Record): SimpleKeyMap { const keyMap: Map = new Map(); - for (const [viewKey, fieldSchema] of Object.entries(fields)) { - const storedKey = getStoredKey(viewKey, fieldSchema); - keyMap.set(viewKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) }); + for (const [propertyKey, fieldSchema] of Object.entries(fields)) { + const storedKey = getStoredKey(propertyKey, fieldSchema); + keyMap.set(propertyKey, { storedKey, schema: normalizeFieldSchema(fieldSchema) }); } return keyMap; @@ -167,8 +167,8 @@ function createProxyHandler( // TODO: Although the target is an object literal, it's still worthwhile to try experimenting with // a dispatch object to see if it improves performance. const handler: ProxyHandler = { - get(target, viewKey, proxy): unknown { - const fieldInfo = flexKeyMap.get(viewKey); + get(target, propertyKey, proxy): unknown { + const fieldInfo = flexKeyMap.get(propertyKey); if (fieldInfo !== undefined) { const flexNode = getOrCreateInnerNode(proxy); @@ -190,13 +190,15 @@ function createProxyHandler( } // Pass the proxy as the receiver here, so that any methods on the prototype receive `proxy` as `this`. - return Reflect.get(target, viewKey, proxy); + return Reflect.get(target, propertyKey, proxy); }, - set(target, viewKey, value: InsertableContent | undefined, proxy) { - const fieldInfo = flexKeyMap.get(viewKey); + set(target, propertyKey, value: InsertableContent | undefined, proxy) { + const fieldInfo = flexKeyMap.get(propertyKey); if (fieldInfo === undefined) { // Pass the proxy as the receiver here, so that setters on the prototype receive `proxy` as `this`. - return allowAdditionalProperties ? Reflect.set(target, viewKey, value, proxy) : false; + return allowAdditionalProperties + ? Reflect.set(target, propertyKey, value, proxy) + : false; } setField( @@ -206,16 +208,16 @@ function createProxyHandler( ); return true; }, - deleteProperty(target, viewKey): boolean { + deleteProperty(target, propertyKey): boolean { // TODO: supporting delete when it makes sense (custom local fields, and optional field) could be added as a feature in the future. throw new UsageError( `Object nodes do not support the delete operator. Optional fields can be assigned to undefined instead.`, ); }, - has: (target, viewKey) => { + has: (target, propertyKey) => { return ( - flexKeyMap.has(viewKey) || - (allowAdditionalProperties ? Reflect.has(target, viewKey) : false) + flexKeyMap.has(propertyKey) || + (allowAdditionalProperties ? Reflect.has(target, propertyKey) : false) ); }, ownKeys: (target) => { @@ -224,12 +226,12 @@ function createProxyHandler( ...(allowAdditionalProperties ? Reflect.ownKeys(target) : []), ]; }, - getOwnPropertyDescriptor: (target, viewKey) => { - const fieldInfo = flexKeyMap.get(viewKey); + getOwnPropertyDescriptor: (target, propertyKey) => { + const fieldInfo = flexKeyMap.get(propertyKey); if (fieldInfo === undefined) { return allowAdditionalProperties - ? Reflect.getOwnPropertyDescriptor(target, viewKey) + ? Reflect.getOwnPropertyDescriptor(target, propertyKey) : undefined; } @@ -310,11 +312,11 @@ export function objectSchema< info: T, implicitlyConstructable: ImplicitlyConstructable, ): ObjectNodeSchema & ObjectNodeSchemaInternalData { - // Ensure no collisions between final set of view keys, and final set of stored keys (including those - // implicitly derived from view keys) + // Ensure no collisions between final set of property keys, and final set of stored keys (including those + // implicitly derived from property keys) assertUniqueKeys(identifier, info); - // Performance optimization: cache view key => stored key and schema. + // Performance optimization: cache property key => stored key and schema. const flexKeyMap: SimpleKeyMap = createFlexKeyMapping(info); let handler: ProxyHandler; @@ -434,8 +436,8 @@ export function objectSchema< const targetToProxy: WeakMap = new WeakMap(); /** - * Ensures that the set of view keys in the schema is unique. - * Also ensure that the final set of stored keys (including those implicitly derived from view keys) is unique. + * Ensures that the set of property keys in the schema is unique. + * Also ensure that the final set of stored keys (including those implicitly derived from property keys) is unique. * @throws Throws a `UsageError` if either of the key uniqueness invariants is violated. */ function assertUniqueKeys< @@ -458,10 +460,10 @@ function assertUniqueKeys< } // Verify that there are no duplicates among the derived - // (including those implicitly derived from view keys) stored keys. + // (including those implicitly derived from property keys) stored keys. const derivedStoredKeys = new Set(); - for (const [viewKey, schema] of Object.entries(fields)) { - const storedKey = getStoredKey(viewKey, schema); + for (const [propertyKey, schema] of Object.entries(fields)) { + const storedKey = getStoredKey(propertyKey, schema); if (derivedStoredKeys.has(storedKey)) { throw new UsageError( `Stored key "${storedKey}" in schema "${schemaName}" conflicts with a property key of the same name, which is not overridden by a stored key. The final set of stored keys in an object schema must be unique.`, diff --git a/packages/dds/tree/src/simple-tree/schemaTypes.ts b/packages/dds/tree/src/simple-tree/schemaTypes.ts index dd832f41e9a4..1704e8ebb9aa 100644 --- a/packages/dds/tree/src/simple-tree/schemaTypes.ts +++ b/packages/dds/tree/src/simple-tree/schemaTypes.ts @@ -76,15 +76,15 @@ export enum FieldKind { } /** - * Maps from a view key to its corresponding {@link FieldProps.key | stored key} for the provided + * Maps from a property key to its corresponding {@link FieldProps.key | stored key} for the provided * {@link ImplicitFieldSchema | field schema}. * * @remarks * If an explicit stored key was specified in the schema, it will be used. - * Otherwise, the stored key is the same as the view key. + * Otherwise, the stored key is the same as the property key. */ -export function getStoredKey(viewKey: string, fieldSchema: ImplicitFieldSchema): FieldKey { - return brand(getExplicitStoredKey(fieldSchema) ?? viewKey); +export function getStoredKey(propertyKey: string, fieldSchema: ImplicitFieldSchema): FieldKey { + return brand(getExplicitStoredKey(fieldSchema) ?? propertyKey); } /** diff --git a/packages/dds/tree/src/simple-tree/simpleSchema.ts b/packages/dds/tree/src/simple-tree/simpleSchema.ts index af3417b91b68..d46a50cf038d 100644 --- a/packages/dds/tree/src/simple-tree/simpleSchema.ts +++ b/packages/dds/tree/src/simple-tree/simpleSchema.ts @@ -28,7 +28,7 @@ export interface SimpleNodeSchemaBase { */ export interface SimpleObjectNodeSchema extends SimpleNodeSchemaBase { /** - * Schemas for each of the object's fields, keyed off of schema's view keys. + * Schemas for each of the object's fields, keyed off of schema's property keys. */ readonly fields: Record; } diff --git a/packages/dds/tree/src/simple-tree/toFlexSchema.ts b/packages/dds/tree/src/simple-tree/toFlexSchema.ts index fec7a65e2c97..e962ebe2e05f 100644 --- a/packages/dds/tree/src/simple-tree/toFlexSchema.ts +++ b/packages/dds/tree/src/simple-tree/toFlexSchema.ts @@ -192,10 +192,10 @@ export function convertNodeSchema( case NodeKind.Object: { const info = schema.info as Record; const fields: Record = Object.create(null); - for (const [viewKey, implicitFieldSchema] of Object.entries(info)) { + for (const [propertyKey, implicitFieldSchema] of Object.entries(info)) { // If a `stored key` was provided, use it as the key in the flex schema. - // Otherwise, use the view key. - const flexKey = getStoredKey(viewKey, implicitFieldSchema); + // Otherwise, use the property key. + const flexKey = getStoredKey(propertyKey, implicitFieldSchema); // This code has to be careful to avoid assigning to __proto__ or similar built-in fields. Object.defineProperty(fields, flexKey, { diff --git a/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts b/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts index 532280f85cee..f0055352f45b 100644 --- a/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts +++ b/packages/dds/tree/src/test/simple-tree/api/schemaFactory.spec.ts @@ -324,7 +324,7 @@ describe("schemaFactory", () => { ); }); - it("Stored key collides with view key", () => { + it("Stored key collides with property key", () => { const schema = new SchemaFactory("com.example"); assert.throws( () => @@ -342,7 +342,7 @@ describe("schemaFactory", () => { // This is a somewhat neurotic test case, and likely not something we would expect a user to do. // But just in case, we should ensure it is handled correctly. - it("Stored key / view key swap", () => { + it("Stored key / property key swap", () => { const schema = new SchemaFactory("com.example"); assert.doesNotThrow(() => schema.object("Object", { @@ -352,7 +352,7 @@ describe("schemaFactory", () => { ); }); - it("Explicit stored key === view key", () => { + it("Explicit stored key === property key", () => { const schema = new SchemaFactory("com.example"); assert.doesNotThrow(() => schema.object("Object", { 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 49bf12a86667..c552f4d5cee9 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 @@ -854,7 +854,7 @@ describe("treeNodeApi", () => { assert.deepEqual(eventLog, [undefined]); }); - it(`'nodeChanged' uses view keys, not stored keys, for the list of changed properties`, () => { + 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" }), From 79c15989de12acc8c3641ecf4c26e031f895f4f9 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 3 Sep 2024 11:22:34 -0700 Subject: [PATCH 31/34] Update APi reports --- packages/dds/tree/api-report/tree.alpha.api.md | 4 ++-- packages/dds/tree/api-report/tree.beta.api.md | 4 ++-- packages/dds/tree/api-report/tree.public.api.md | 4 ++-- .../fluid-framework/api-report/fluid-framework.beta.api.md | 4 ++-- .../api-report/fluid-framework.legacy.alpha.api.md | 4 ++-- .../api-report/fluid-framework.legacy.public.api.md | 4 ++-- .../fluid-framework/api-report/fluid-framework.public.api.md | 4 ++-- 7 files changed, 14 insertions(+), 14 deletions(-) diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 380a778a2b33..9b930e4231e5 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -286,7 +286,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -478,7 +478,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 397af8e10d8d..91ce14d897e9 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -218,7 +218,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -407,7 +407,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index 0ee4443d8b1e..fcc1521cd61f 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -218,7 +218,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -407,7 +407,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } 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 20adc8f79a3d..2b2b33999d61 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 @@ -563,7 +563,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -779,7 +779,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } 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 0f0e2e9bdf01..7ea11f77a31a 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 @@ -869,7 +869,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -1176,7 +1176,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } 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 4c7afea67ab0..fde0fcb0a566 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 @@ -599,7 +599,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -819,7 +819,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } 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 83109ae05a1e..9b83ad7db7c2 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 @@ -563,7 +563,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public +// @public @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -779,7 +779,7 @@ export interface TreeArrayNodeUnsafe { +export interface TreeChangeEvents { nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; treeChanged(): void; } From cf73e4da244b8dc99ea418a8fbda1e28a52d0d44 Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Tue, 3 Sep 2024 11:50:57 -0700 Subject: [PATCH 32/34] Make new event API beta --- .../dds/tree/api-report/tree.alpha.api.md | 18 +++- packages/dds/tree/api-report/tree.beta.api.md | 18 +++- .../dds/tree/api-report/tree.public.api.md | 11 +-- packages/dds/tree/src/index.ts | 3 + .../dds/tree/src/simple-tree/api/index.ts | 1 + .../tree/src/simple-tree/api/treeApiBeta.ts | 94 +++++++++++++++++++ .../tree/src/simple-tree/api/treeNodeApi.ts | 6 +- .../dds/tree/src/simple-tree/core/index.ts | 1 - .../dds/tree/src/simple-tree/core/types.ts | 29 +----- packages/dds/tree/src/simple-tree/index.ts | 4 +- .../src/test/simple-tree/api/treeApi.spec.ts | 9 +- .../api-report/fluid-framework.beta.api.md | 11 +-- .../fluid-framework.legacy.alpha.api.md | 11 +-- .../fluid-framework.legacy.public.api.md | 11 +-- .../api-report/fluid-framework.public.api.md | 11 +-- 15 files changed, 156 insertions(+), 82 deletions(-) create mode 100644 packages/dds/tree/src/simple-tree/api/treeApiBeta.ts diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 9b930e4231e5..6f6ef1d18925 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -286,7 +286,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed +// @beta @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -477,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: TreeChangeEventsBeta[K]) => () => void; +}; + // @public @sealed -export interface TreeChangeEvents { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } +// @beta @sealed +export interface TreeChangeEventsBeta extends TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +} + // @public export type TreeFieldFromImplicitField = TSchema extends FieldSchema ? ApplyKind, Kind, false> : TSchema extends ImplicitAllowedTypes ? TreeNodeFromImplicitAllowedTypes : unknown; @@ -522,7 +532,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/api-report/tree.beta.api.md b/packages/dds/tree/api-report/tree.beta.api.md index 91ce14d897e9..e3fe006dcd93 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -218,7 +218,7 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed +// @beta @sealed export interface NodeChangedData { readonly changedProperties?: ReadonlySet; } @@ -406,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: TreeChangeEventsBeta[K]) => () => void; +}; + // @public @sealed -export interface TreeChangeEvents { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } +// @beta @sealed +export interface TreeChangeEventsBeta extends TreeChangeEvents { + nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +} + // @public export type TreeFieldFromImplicitField = TSchema extends FieldSchema ? ApplyKind, Kind, false> : TSchema extends ImplicitAllowedTypes ? TreeNodeFromImplicitAllowedTypes : unknown; @@ -451,7 +461,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/api-report/tree.public.api.md b/packages/dds/tree/api-report/tree.public.api.md index fcc1521cd61f..438b50d8ec65 100644 --- a/packages/dds/tree/api-report/tree.public.api.md +++ b/packages/dds/tree/api-report/tree.public.api.md @@ -218,11 +218,6 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; -} - // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -407,8 +402,8 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } @@ -451,7 +446,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; diff --git a/packages/dds/tree/src/index.ts b/packages/dds/tree/src/index.ts index 0baf19254a22..6e5e0f3ee0d2 100644 --- a/packages/dds/tree/src/index.ts +++ b/packages/dds/tree/src/index.ts @@ -125,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..41c9272a5373 --- /dev/null +++ b/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts @@ -0,0 +1,94 @@ +/*! + * 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; +} + +/** + * 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). + */ + nodeChanged( + data: NodeChangedData & + // For object and Map nodes, make properties specific to them required instead of optional: + (TNode extends WithType + ? Required> + : 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: TreeChangeEventsBeta[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 c49cca30e0b7..b368a75c3815 100644 --- a/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts +++ b/packages/dds/tree/src/simple-tree/api/treeNodeApi.ts @@ -99,10 +99,10 @@ export interface TreeNodeApi { * @returns A callback function which will deregister the event. * This callback should be called only once. */ - on, TNode extends TreeNode>( - node: TNode, + on( + node: TreeNode, eventName: K, - listener: TreeChangeEvents[K], + listener: TreeChangeEvents[K], ): () => void; /** diff --git a/packages/dds/tree/src/simple-tree/core/index.ts b/packages/dds/tree/src/simple-tree/core/index.ts index 952e8165fb2c..16fcbc12e406 100644 --- a/packages/dds/tree/src/simple-tree/core/index.ts +++ b/packages/dds/tree/src/simple-tree/core/index.ts @@ -17,7 +17,6 @@ export { inPrototypeChain, type InternalTreeNode, privateToken, - type NodeChangedData, } from "./types.js"; export { type TreeNodeSchema, diff --git a/packages/dds/tree/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index 02f1de99d100..8f1d47ef764a 100644 --- a/packages/dds/tree/src/simple-tree/core/types.ts +++ b/packages/dds/tree/src/simple-tree/core/types.ts @@ -24,23 +24,6 @@ import { isFlexTreeNode, type FlexTreeNode } from "../../feature-libraries/index */ export type Unhydrated = T; -/** - * Data included for {@link TreeChangeEvents.nodeChanged}. - * @sealed @public - */ -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; -} - /** * A collection of events that can be emitted by a {@link TreeNode}. * @@ -67,7 +50,7 @@ export interface NodeChangedData { * * @sealed @public */ -export interface TreeChangeEvents { +export interface TreeChangeEvents { /** * Emitted by a node after a batch of changes has been applied to the tree, if any of the changes affected the node. * @@ -77,6 +60,8 @@ export interface TreeChangeEvents { * * - Map nodes define a change as when an entry is added, updated, or removed. * + * @param unstable - Unspecified data which may get defined/specified in future versions of this API. + * * @remarks * This event is not emitted when: * @@ -100,13 +85,7 @@ export interface TreeChangeEvents { * * TODO: define and document event ordering (ex: bottom up, with nodeChanged before treeChange on each level). */ - nodeChanged( - data: NodeChangedData & - // For object and Map nodes, make properties specific to them required instead of optional: - (TNode extends WithType - ? Required> - : unknown), - ): 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/index.ts b/packages/dds/tree/src/simple-tree/index.ts index f4154f73c8f1..681653d3a828 100644 --- a/packages/dds/tree/src/simple-tree/index.ts +++ b/packages/dds/tree/src/simple-tree/index.ts @@ -19,7 +19,6 @@ export { type Unhydrated, type InternalTreeNode, isTreeNode, - type NodeChangedData, } from "./core/index.js"; export { type ITree, @@ -45,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/test/simple-tree/api/treeApi.spec.ts b/packages/dds/tree/src/test/simple-tree/api/treeApi.spec.ts index de63f2700db7..5bf5170779b3 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,6 +19,7 @@ import { type NodeFromSchema, SchemaFactory, treeNodeApi as Tree, + TreeBeta, type TreeChangeEvents, TreeViewConfiguration, } from "../../../simple-tree/index.js"; @@ -781,7 +782,7 @@ describe("treeNodeApi", () => { const root = view.root; const eventLog: ReadonlySet[] = []; - Tree.on(root, "nodeChanged", ({ changedProperties }) => + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => eventLog.push(changedProperties), ); @@ -812,7 +813,7 @@ describe("treeNodeApi", () => { const root = view.root; const eventLog: ReadonlySet[] = []; - Tree.on(root, "nodeChanged", ({ changedProperties }) => + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => eventLog.push(changedProperties), ); @@ -838,7 +839,7 @@ describe("treeNodeApi", () => { const root = view.root; const eventLog: (ReadonlySet | undefined)[] = []; - Tree.on(root, "nodeChanged", (data) => eventLog.push(data.changedProperties)); + TreeBeta.on(root, "nodeChanged", (data) => eventLog.push(data.changedProperties)); const { forkView, forkCheckout } = getViewForForkedBranch(view); @@ -864,7 +865,7 @@ describe("treeNodeApi", () => { const root = view.root; const eventLog: ReadonlySet[] = []; - Tree.on(root, "nodeChanged", ({ changedProperties }) => + TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => eventLog.push(changedProperties), ); 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 2b2b33999d61..cd7ccf8f03eb 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 @@ -563,11 +563,6 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; -} - // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -779,8 +774,8 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } @@ -823,7 +818,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 7ea11f77a31a..0972a7ef0100 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 @@ -869,11 +869,6 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; -} - // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -1176,8 +1171,8 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } @@ -1220,7 +1215,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 fde0fcb0a566..d559c38cb86d 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 @@ -599,11 +599,6 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; -} - // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -819,8 +814,8 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } @@ -863,7 +858,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; 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 9b83ad7db7c2..d42d83c7c726 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 @@ -563,11 +563,6 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; -// @public @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; -} - // @public export type NodeFromSchema = T extends TreeNodeSchema ? TNode : never; @@ -779,8 +774,8 @@ export interface TreeArrayNodeUnsafe { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; +export interface TreeChangeEvents { + nodeChanged(unstable?: unknown): void; treeChanged(): void; } @@ -823,7 +818,7 @@ export abstract class TreeNode implements WithType { export interface TreeNodeApi { is(value: unknown, schema: TSchema): value is TreeNodeFromImplicitAllowedTypes; key(node: TreeNode): string | number; - on, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEvents[K]): () => void; + on(node: TreeNode, eventName: K, listener: TreeChangeEvents[K]): () => void; parent(node: TreeNode): TreeNode | undefined; schema(node: TreeNode | TreeLeafValue): TreeNodeSchema; shortId(node: TreeNode): number | string | undefined; From 3363b004152ee4bfde62cd49d4700379527164fd Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 4 Sep 2024 15:44:14 -0700 Subject: [PATCH 33/34] Add strongly typed keys --- .changeset/wet-hoops-drop.md | 33 +++++- .../dds/tree/api-report/tree.alpha.api.md | 18 +-- packages/dds/tree/api-report/tree.beta.api.md | 18 +-- .../dds/tree/api-report/tree.public.api.md | 10 +- .../tree/src/simple-tree/api/treeApiBeta.ts | 21 ++-- .../dds/tree/src/simple-tree/core/withType.ts | 4 +- .../dds/tree/src/simple-tree/objectNode.ts | 2 +- .../src/test/simple-tree/api/treeApi.spec.ts | 107 +++++++++++++++++- .../api-report/fluid-framework.beta.api.md | 10 +- .../fluid-framework.legacy.alpha.api.md | 10 +- .../fluid-framework.legacy.public.api.md | 10 +- .../api-report/fluid-framework.public.api.md | 10 +- 12 files changed, 194 insertions(+), 59 deletions(-) diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md index 5b737be12d49..86e1c2436450 100644 --- a/.changeset/wet-hoops-drop.md +++ b/.changeset/wet-hoops-drop.md @@ -3,9 +3,27 @@ "@fluidframework/tree": minor --- -nodeChanged event now includes the list of properties that changed -The payload of the `nodeChanged` event emitted by SharedTree now includes a `changedProperties` property that indicates +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 @@ -15,3 +33,14 @@ See the documentation for `FieldProps` for more details about the distinction be 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")) { + // ... + } +}); +``` diff --git a/packages/dds/tree/api-report/tree.alpha.api.md b/packages/dds/tree/api-report/tree.alpha.api.md index 6f6ef1d18925..7238a2b10548 100644 --- a/packages/dds/tree/api-report/tree.alpha.api.md +++ b/packages/dds/tree/api-report/tree.alpha.api.md @@ -287,8 +287,8 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; // @beta @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet ? string & keyof TInfo : string>; } // @public @@ -397,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>; @@ -405,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 @@ -479,7 +479,7 @@ export interface TreeArrayNodeUnsafe, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEventsBeta[K]) => () => void; + readonly on: , TNode extends TreeNode>(node: TNode, eventName: K, listener: NoInfer[K]>) => () => void; }; // @public @sealed @@ -490,7 +490,7 @@ export interface TreeChangeEvents { // @beta @sealed export interface TreeChangeEventsBeta extends TreeChangeEvents { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; + nodeChanged: (data: NodeChangedData & (TNode extends WithType ? Required, "changedProperties">> : unknown)) => void; } // @public @@ -570,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; @@ -634,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 e3fe006dcd93..d7b65e514228 100644 --- a/packages/dds/tree/api-report/tree.beta.api.md +++ b/packages/dds/tree/api-report/tree.beta.api.md @@ -219,8 +219,8 @@ type NodeBuilderData = T extends TreeNodeSchema> = T extends TreeNodeSchema ? TBuild : never; // @beta @sealed -export interface NodeChangedData { - readonly changedProperties?: ReadonlySet; +export interface NodeChangedData { + readonly changedProperties?: ReadonlySet ? string & keyof TInfo : string>; } // @public @@ -329,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>; @@ -337,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 @@ -408,7 +408,7 @@ export interface TreeArrayNodeUnsafe, TNode extends TreeNode>(node: TNode, eventName: K, listener: TreeChangeEventsBeta[K]) => () => void; + readonly on: , TNode extends TreeNode>(node: TNode, eventName: K, listener: NoInfer[K]>) => () => void; }; // @public @sealed @@ -419,7 +419,7 @@ export interface TreeChangeEvents { // @beta @sealed export interface TreeChangeEventsBeta extends TreeChangeEvents { - nodeChanged(data: NodeChangedData & (TNode extends WithType ? Required> : unknown)): void; + nodeChanged: (data: NodeChangedData & (TNode extends WithType ? Required, "changedProperties">> : unknown)) => void; } // @public @@ -499,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; @@ -563,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 438b50d8ec65..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 @@ -484,7 +484,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 +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/simple-tree/api/treeApiBeta.ts b/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts index 41c9272a5373..956bce801e7d 100644 --- a/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts +++ b/packages/dds/tree/src/simple-tree/api/treeApiBeta.ts @@ -10,7 +10,7 @@ import { treeNodeApi } from "./treeNodeApi.js"; * Data included for {@link TreeChangeEventsBeta.nodeChanged}. * @sealed @beta */ -export interface NodeChangedData { +export interface NodeChangedData { /** * When the node changed is an object or Map node, this lists all the properties which changed. * @remarks @@ -20,7 +20,12 @@ export interface NodeChangedData { * * 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; + 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 + >; } /** @@ -61,14 +66,16 @@ export interface TreeChangeEventsBeta * (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 & + nodeChanged: ( + data: NodeChangedData & // For object and Map nodes, make properties specific to them required instead of optional: (TNode extends WithType - ? Required> + ? Required, "changedProperties">> : unknown), - ): void; + ) => void; } /** @@ -87,7 +94,7 @@ export const TreeBeta = { on, TNode extends TreeNode>( node: TNode, eventName: K, - listener: TreeChangeEventsBeta[K], + listener: NoInfer[K]>, ): () => void { return treeNodeApi.on(node, eventName, listener); }, 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/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 5bf5170779b3..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 @@ -21,11 +21,12 @@ import { treeNodeApi as Tree, TreeBeta, type TreeChangeEvents, + type TreeNode, TreeViewConfiguration, } from "../../../simple-tree/index.js"; import { getView } from "../../utils.js"; import { getViewForForkedBranch, hydrate } from "../utils.js"; -import { brand } from "../../../util/index.js"; +import { brand, type areSafelyAssignable, type requireTrue } from "../../../util/index.js"; import { booleanSchema, @@ -781,10 +782,11 @@ describe("treeNodeApi", () => { view.initialize({ prop1: 1, prop2: 2 }); const root = view.root; - const eventLog: ReadonlySet[] = []; - TreeBeta.on(root, "nodeChanged", ({ changedProperties }) => - eventLog.push(changedProperties), - ); + // 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); @@ -799,6 +801,101 @@ describe("treeNodeApi", () => { 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]) {} 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 cd7ccf8f03eb..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 @@ -856,7 +856,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; @@ -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 0972a7ef0100..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 @@ -1253,7 +1253,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; @@ -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 d559c38cb86d..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 @@ -896,7 +896,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; @@ -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 d42d83c7c726..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 @@ -856,7 +856,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; @@ -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; } ``` From 65b0b6ab690b6deea9923e4126cae33ca96bb9ef Mon Sep 17 00:00:00 2001 From: "Craig Macomber (Microsoft)" <42876482+CraigMacomber@users.noreply.github.com> Date: Wed, 4 Sep 2024 17:09:34 -0700 Subject: [PATCH 34/34] document unstable --- .changeset/wet-hoops-drop.md | 20 +++++++++++++++++++ .../dds/tree/src/simple-tree/core/types.ts | 5 +++++ 2 files changed, 25 insertions(+) diff --git a/.changeset/wet-hoops-drop.md b/.changeset/wet-hoops-drop.md index 86e1c2436450..fd1a4e3c686c 100644 --- a/.changeset/wet-hoops-drop.md +++ b/.changeset/wet-hoops-drop.md @@ -44,3 +44,23 @@ TreeBeta.on(point, "nodeChanged", (data) => { } }); ``` + +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/src/simple-tree/core/types.ts b/packages/dds/tree/src/simple-tree/core/types.ts index ac859e6c80ca..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: *