Skip to content

Commit

Permalink
tree-comparison demo updates part 2 (#17852)
Browse files Browse the repository at this point in the history 
Continuation of #17735 and #17774
  • Loading branch information
@ChumpChief
ChumpChief authored Oct 25, 2023
1 parent f6d8fee commit 105c818
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 111 deletions.
49 changes: 0 additions & 49 deletions examples/apps/tree-comparison/src/model/inventoryItem.ts
View file

This file was deleted.

61 changes: 56 additions & 5 deletions examples/apps/tree-comparison/src/model/legacyTreeInventoryList.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<IInventoryItemEvents>
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<string, InventoryItem>();
private readonly _inventoryItems = new Map<string, LegacyTreeInventoryItem>();

private get tree() {
if (this._tree === undefined) {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
}

Expand Down
139 changes: 82 additions & 57 deletions examples/apps/tree-comparison/src/model/newTreeInventoryList.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof inventoryItemSchema>;

// 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<typeof inventorySchema>;

// 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<typeof inventoryFieldSchema>;

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<IInventoryItemEvents> 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 {
Expand All @@ -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<string, InventoryItem>();
private readonly _inventoryItems = new Map<string, NewTreeInventoryItem>();

public readonly addItem = (name: string, quantity: number) => {
this.inventory.inventoryItems.insertAtEnd([
Expand All @@ -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<void> {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
this._sharedTree = await this.root
Expand All @@ -107,8 +148,12 @@ export class NewTreeInventoryList extends DataObject implements IInventoryList {
}

protected async hasInitialized(): Promise<void> {
// 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: [
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
}

Expand Down

0 comments on commit 105c818

Please sign in to comment.