From 105c818220af175536c394452ad8b4f2a4904d25 Mon Sep 17 00:00:00 2001 From: Matt Rakow Date: Wed, 25 Oct 2023 10:48:24 -0700 Subject: [PATCH] tree-comparison demo updates part 2 (#17852) Continuation of #17735 and #17774 --- .../src/model/inventoryItem.ts | 49 ------ .../src/model/legacyTreeInventoryList.ts | 61 +++++++- .../src/model/newTreeInventoryList.ts | 139 +++++++++++------- 3 files changed, 138 insertions(+), 111 deletions(-) delete mode 100644 examples/apps/tree-comparison/src/model/inventoryItem.ts diff --git a/examples/apps/tree-comparison/src/model/inventoryItem.ts b/examples/apps/tree-comparison/src/model/inventoryItem.ts deleted file mode 100644 index d273b82c47bd..000000000000 --- a/examples/apps/tree-comparison/src/model/inventoryItem.ts +++ /dev/null @@ -1,49 +0,0 @@ -/*! - * Copyright (c) Microsoft Corporation and contributors. All rights reserved. - * Licensed under the MIT License. - */ - -import { TypedEmitter } from "tiny-typed-emitter"; - -import type { IInventoryItem, IInventoryItemEvents } from "../modelInterfaces"; - -/** - * InventoryItem is the local object with the friendly interface for the view to use. - * It binds to either legacy or new SharedTree by abstracting out how the values are - * changed. - */ -export class InventoryItem extends TypedEmitter implements IInventoryItem { - public get id() { - return this._id; - } - public get name() { - return this._name; - } - public get quantity() { - return this._quantity; - } - public set quantity(newQuantity: number) { - // Setting the quantity does not directly update the value, but rather roundtrips it through - // the backing data by using the provided callback. We trust that later this will result in - // handleQuantityUpdate getting called when the true backing data changes. - this._setQuantity(newQuantity); - } - /** - * handleQuantityUpdate is not available on IInventoryItem intentionally, since it should not be - * available to the view. Instead it is to be called by the backing data when the true value - * of the data changes. - */ - public handleQuantityUpdate(newQuantity: number) { - this._quantity = newQuantity; - this.emit("quantityChanged"); - } - public constructor( - private readonly _id: string, - private readonly _name: string, - private _quantity: number, - private readonly _setQuantity: (quantity: number) => void, - public readonly deleteItem: () => void, - ) { - super(); - } -} diff --git a/examples/apps/tree-comparison/src/model/legacyTreeInventoryList.ts b/examples/apps/tree-comparison/src/model/legacyTreeInventoryList.ts index f3b26c51f09f..761eea765ab2 100644 --- a/examples/apps/tree-comparison/src/model/legacyTreeInventoryList.ts +++ b/examples/apps/tree-comparison/src/model/legacyTreeInventoryList.ts @@ -16,16 +16,60 @@ import { } from "@fluid-experimental/tree"; import { DataObject, DataObjectFactory } from "@fluidframework/aqueduct"; import { IFluidHandle } from "@fluidframework/core-interfaces"; +import { TypedEmitter } from "tiny-typed-emitter"; import { v4 as uuid } from "uuid"; -import type { IInventoryItem, IInventoryList } from "../modelInterfaces"; -import { InventoryItem } from "./inventoryItem"; +import type { IInventoryItem, IInventoryItemEvents, IInventoryList } from "../modelInterfaces"; const legacySharedTreeKey = "legacySharedTree"; +/** + * LegacyTreeInventoryItem is the local object with a friendly interface for the view to use. + * The LegacyTreeInventoryList can bind these to its inventory items to abstract out how the values are + * changed. + */ +export class LegacyTreeInventoryItem + extends TypedEmitter + implements IInventoryItem +{ + public get id() { + return this._id; + } + public get name() { + return this._name; + } + public get quantity() { + return this._quantity; + } + public set quantity(newQuantity: number) { + // Setting the quantity does not directly update the value, but rather roundtrips it through + // the backing data by using the provided callback. We trust that later this will result in + // handleQuantityUpdate getting called when the true backing data changes. + this._setQuantity(newQuantity); + } + /** + * handleQuantityUpdate is not available on IInventoryItem intentionally, since it should not be + * available to the view. Instead it is to be called by the backing data when the true value + * of the data changes. + */ + public handleQuantityUpdate(newQuantity: number) { + this._quantity = newQuantity; + this.emit("quantityChanged"); + } + public constructor( + private readonly _id: string, + private readonly _name: string, + private _quantity: number, + private readonly _setQuantity: (quantity: number) => void, + public readonly deleteItem: () => void, + ) { + super(); + } +} + export class LegacyTreeInventoryList extends DataObject implements IInventoryList { private _tree: LegacySharedTree | undefined; - private readonly _inventoryItems = new Map(); + private readonly _inventoryItems = new Map(); private get tree() { if (this._tree === undefined) { @@ -214,7 +258,14 @@ export class LegacyTreeInventoryList extends DataObject implements IInventoryLis } } - private makeInventoryItemFromInventoryItemNode(inventoryItemNode: TreeViewNode): InventoryItem { + // Note that because accessing and modifying the legacy SharedTree depends so much on access to the tree itself + // (in order to have access to tree.currentView.getViewNode and tree.applyEdit) it's more difficult to encapsulate + // an individual inventory item with its corresponding subtree. Here I prefer to pass callbacks wrapping that + // access to the LegacyTreeInventoryItem rather than hand out access to the whole tree, whereas NewTreeInventoryItem + // can do most of its work with just the item node. + private makeInventoryItemFromInventoryItemNode( + inventoryItemNode: TreeViewNode, + ): LegacyTreeInventoryItem { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion const idNodeId = inventoryItemNode.traits.get("id" as TraitLabel)![0]; const idNode = this.tree.currentView.getViewNode(idNodeId); @@ -238,7 +289,7 @@ export class LegacyTreeInventoryList extends DataObject implements IInventoryLis this.tree.applyEdit(Change.delete(StableRange.only(inventoryItemNode.identifier))); }; - return new InventoryItem(id, name, quantity, setQuantity, deleteItem); + return new LegacyTreeInventoryItem(id, name, quantity, setQuantity, deleteItem); } } diff --git a/examples/apps/tree-comparison/src/model/newTreeInventoryList.ts b/examples/apps/tree-comparison/src/model/newTreeInventoryList.ts index a14590370a83..f26782417b46 100644 --- a/examples/apps/tree-comparison/src/model/newTreeInventoryList.ts +++ b/examples/apps/tree-comparison/src/model/newTreeInventoryList.ts @@ -14,48 +14,84 @@ import { } from "@fluid-experimental/tree2"; import { DataObject, DataObjectFactory } from "@fluidframework/aqueduct"; import { IFluidHandle } from "@fluidframework/core-interfaces"; +import { TypedEmitter } from "tiny-typed-emitter"; import { v4 as uuid } from "uuid"; -import type { IInventoryItem, IInventoryList } from "../modelInterfaces"; -import { InventoryItem } from "./inventoryItem"; +import type { IInventoryItem, IInventoryItemEvents, IInventoryList } from "../modelInterfaces"; +// To define the tree schema, we'll make a series of calls to a SchemaBuilder to produce schema objects. +// The final schema object will later be used as an argument to the schematize call. AB#5967 const builder = new SchemaBuilder({ scope: "inventory app" }); const inventoryItemSchema = builder.object("Contoso:InventoryItem-1.0.0", { + // Some unique identifier appropriate for the inventory scenario (e.g. a UPC or model number) id: builder.string, + // A user-friendly name name: builder.string, + // The number in stock quantity: builder.number, }); type InventoryItemNode = Typed; -// REV: Building this up as a series of builder invocations makes it hard to read the schema. -// Would be nice if instead we could define some single big Serializable or similar that laid the -// schema out and then pass that in. // TODO: Convert this to use builder.list() rather than builder.sequence when ready. const inventorySchema = builder.object("Contoso:Inventory-1.0.0", { inventoryItems: builder.sequence(inventoryItemSchema), }); type InventoryNode = Typed; -// REV: The root inventoryFieldSchema feels extra to me. Is there a way to omit it? Something like -// builder.intoSchema(inventorySchema) -const inventoryFieldSchema = SchemaBuilder.required(inventorySchema); -type InventoryField = Typed; - -const schema = builder.intoSchema(inventoryFieldSchema); +// This call finalizes the schema into an object we can pass to schematize. +const schema = builder.intoSchema(inventorySchema); const newTreeFactory = new TypedTreeFactory({ jsonValidator: typeboxValidator, - // REV: I copied this from another example but I have no idea what it means - documentation is - // self-referencing. + // For now, ignore the forest argument - I think it's probably going away once the optimized one is ready anyway? AB#6013 forest: ForestType.Reference, - // REV: What's the scenario where I'd want to leverage the subtype? Documentation makes it sound - // like it should be optional at least. + // For now, ignore the subtype. However, be aware that it is written into the document and so must remain consistent + // in order to load existing documents. AB#6014 subtype: "InventoryList", }); const sharedTreeKey = "sharedTree"; +/** + * NewTreeInventoryItem is the local object with a friendly interface for the view to use. + * It wraps a new SharedTree node representing an inventory item to abstract out the tree manipulation and access. + */ +class NewTreeInventoryItem extends TypedEmitter implements IInventoryItem { + private readonly _unregisterChangingEvent: () => void; + public get id() { + return this._inventoryItemNode.id; + } + public get name() { + return this._inventoryItemNode.name; + } + public get quantity() { + return this._inventoryItemNode.quantity; + } + public set quantity(newQuantity: number) { + // Note that modifying the content here is actually changing the data stored in the SharedTree legitimately + // (i.e. results in an op sent and changes reflected on all clients). AB#5970 + this._inventoryItemNode.boxedQuantity.content = newQuantity; + } + public constructor( + private readonly _inventoryItemNode: InventoryItemNode, + private readonly _removeItemFromTree: () => void, + ) { + super(); + // Note that this is not a normal Node EventEmitter and functions differently. There is no "off" method, + // but instead "on" returns a callback to unregister the event. AB#5973 + this._unregisterChangingEvent = this._inventoryItemNode.on("changing", () => { + this.emit("quantityChanged"); + }); + } + public readonly deleteItem = () => { + // TODO: Maybe expose a public dispose() method for disposing the NewTreeInventoryItem without + // modifying the tree? + this._unregisterChangingEvent(); + this._removeItemFromTree(); + }; +} + export class NewTreeInventoryList extends DataObject implements IInventoryList { private _sharedTree: TypedTreeChannel | undefined; private get sharedTree(): TypedTreeChannel { @@ -64,14 +100,14 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList { } return this._sharedTree; } - private _inventory: InventoryField | undefined; + private _inventory: InventoryNode | undefined; private get inventory(): InventoryNode { if (this._inventory === undefined) { throw new Error("Not initialized properly"); } - return this._inventory.content; + return this._inventory; } - private readonly _inventoryItems = new Map(); + private readonly _inventoryItems = new Map(); public readonly addItem = (name: string, quantity: number) => { this.inventory.inventoryItems.insertAtEnd([ @@ -95,10 +131,15 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList { newTreeFactory.type, ) as TypedTreeChannel; this.root.set(sharedTreeKey, this._sharedTree.handle); + // Convenient repro for bug AB#5975 + // const retrievedSharedTree = await this._sharedTree.handle.get(); + // if (this._sharedTree !== retrievedSharedTree) { + // console.log(this._sharedTree, retrievedSharedTree); + // throw new Error("handle doesn't roundtrip on initial creation"); + // } } - // REV: Have to use initializingFromExisting here rather than hasInitialized due to a bug - getting - // the handle on the creating client retrieves the wrong object. + // This would usually live in hasInitialized - I'm using initializingFromExisting here due to bug AB#5975. protected async initializingFromExisting(): Promise { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion this._sharedTree = await this.root @@ -107,8 +148,12 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList { } protected async hasInitialized(): Promise { - // REV: I don't particularly like the combined schematize/initialize. My preference would be for - // separate schematize/initialize calls. + // Note that although we always pass initialTree, it's only actually used on the first load and + // is ignored on subsequent loads. AB#5974 + // Note that because we passed an "object" to the toDocumentSchema() call (rather than a RequiredField), + // that call will automatically generate and wrap our object in a RequiredField. That automatically + // generated RequiredField is what we get back from the .schematize() call. So to get back to our + // object type we need to get the .content off of the return value of .schematize(). this._inventory = this.sharedTree.schematize({ initialTree: { inventoryItems: [ @@ -126,30 +171,22 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList { }, allowedSchemaModifications: AllowedUpdateType.None, schema, - }); - // REV: This event feels overly-broad for what I'm looking for, but I'm having issues with - // more node-specific events ("changing", etc.). I also personally find the deviation from - // standard EventEmitter API surprising/unintuitive/inconvenient. + }).content; + // afterChange will fire for any change of any type anywhere in the subtree. In this application we expect + // three types of tree changes that will trigger this handler - add items, delete items, change item quantities. + // Since "afterChange" doesn't provide event args, we need to scan the tree and compare it to our InventoryItems + // to find what changed. We'll intentionally ignore the quantity changes here, which are instead handled by + // "changing" listeners on each individual item node. this.inventory.context.on("afterChange", () => { - // Since "afterChange" doesn't provide event args, we need to scan the tree and compare - // it to our InventoryItems to find what changed. This event handler fires for any - // change to the tree, so it needs to handle all possibilities (change, add, remove). for (const inventoryItemNode of this.inventory.inventoryItems) { - const upToDateQuantity = inventoryItemNode.quantity; - const inventoryItem = this._inventoryItems.get(inventoryItemNode.id); // If we're not currently tracking some item in the tree, then it must have been // added in this change. - if (inventoryItem === undefined) { + if (!this._inventoryItems.has(inventoryItemNode.id)) { const newInventoryItem = this.makeInventoryItemFromInventoryItemNode(inventoryItemNode); this._inventoryItems.set(inventoryItemNode.id, newInventoryItem); this.emit("itemAdded"); } - // If the quantity of our tracking item is different from the tree, then the - // quantity must have changed in this change. - if (inventoryItem !== undefined && inventoryItem.quantity !== upToDateQuantity) { - inventoryItem.handleQuantityUpdate(upToDateQuantity); - } } // Search for deleted inventory items to update our collection @@ -177,28 +214,16 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList { private makeInventoryItemFromInventoryItemNode( inventoryItemNode: InventoryItemNode, - ): InventoryItem { - const setQuantity = (newQuantity: number) => { - // REV: This still seems surprising to me that it's making the remote change, I think - // it would be more apparent as .setContent() rather than using the setter. - inventoryItemNode.boxedQuantity.content = newQuantity; - }; - const deleteItem = () => { - // REV: Is this the best way to do this? Was hoping for maybe just an inventoryItemNode.delete(). + ): NewTreeInventoryItem { + const removeItemFromTree = () => { + // Although it's possible to remove a node from the tree without the parent reference, it gets a little messy: + // (inventoryItemNode.parentField.parent as any).removeAt(inventoryItemNode.parentField.index); + // So for now we'll pass in the delete capability as a callback to withold this.inventory access from the + // inventory items. AB#6015 this.inventory.inventoryItems.removeAt(inventoryItemNode.parentField.index); }; - // REV: Per-node events seem buggy (this fires twice per change, presumably for local change + ack?) - // inventoryItemNode.on("changing", () => { - // console.log(`changing: ${inventoryItemNode.quantity}`); - // inventoryItem.handleQuantityUpdate(inventoryItemNode.quantity); - // }); - return new InventoryItem( - inventoryItemNode.id, - inventoryItemNode.name, - inventoryItemNode.quantity, - setQuantity, - deleteItem, - ); + const inventoryItem = new NewTreeInventoryItem(inventoryItemNode, removeItemFromTree); + return inventoryItem; } }