Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated inventory-app example to show more tree usage #17568

Closed
wants to merge 39 commits into from

Conversation

ChumpChief
Copy link
Contributor

I've been experimenting to understand the legacy and new SharedTree APIs. Since we don't exactly have an intro-level example for these, this PR updates the inventory-app example to show more direct API usage. Hoping y'all can take a look and help correct me if I'm doing things wrong :) I've included REV: comments where I'd particularly like feedback or hit interesting issues.

Screenshot 2023-09-29 at 2 57 39 PM

@ChumpChief ChumpChief requested review from msfluid-bot and a team as code owners September 29, 2023 21:58

export const schemaPolicy = {
schema,
initialTree: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is SharedTree.context.root after schematize is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close - schematize gives an ISharedTreeView (it's not on the SharedTree directly).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so the path from the sharedTree dds (idk if this is a good idea) is SharedTree.SharedTreeView.context.root. That seems to be pretty deep for me. As a user, I would just like something like SharedTree.root or something like that. I'm assuming there's always going to be some sort of root like node

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of - SharedTree.view is actually deprecated so I think the only way to get the view is with a SharedTree.schematize() call which returns an ISharedTreeView. I don't think there's a direct path from SharedTree to its contents like you're looking for. I agree that would be nice though, and had a comment in a similar spirit here.

Comment on lines +107 to +110
// REV: It would probably be preferable to have a durable IPart that raises individual
// "quantityChanged" events rather than eventing/refreshing on a whole-tree basis. Is
// there a good way to listen for tree edits under a specific node? Is the partQuantityNode
// reference durable across tree changes?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to do this relatively efficiently with legacy shared-tree, but the machinery has to be implemented on consumer side. I believe new shared-tree's eventing API does allow scoped subscription.

In terms of durability, node ids are stable across all edits, so the callbacks you're attaching are fine to use across edits and should do what you want, you just won't have all of the right nodes (or too many, if some have been deleted since the last call to getParts()). The view nodes are still valid to call APIs on, but probably aren't what you want (they do not get updated in response to changes--the underlying data structures powering the view are copy-on-write-like)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took another look at per-node eventing on the consumer side - I see that SharedTreeEvent.EditCommitted includes an editId, which I think can then be referenced against the editLog. However I'm not seeing a clean way to go find the modified nodeId if that's the idea - best path I found looks something like:

(sharedTree.edits.tryGetEditFromId(editId)!.changes[0] as unknown as SetValueInternal).nodeToModify

Does that sound like the right call? I can tell I'm doing a few bad things in there (assuming indexing [0] is correct, casting to an internal type), maybe there's a nicer way.

Re: durability: If I'm holding refs to two nodes A and B, and A updates - are both refs now "stale" or is my B ref still good?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't want to use the edit log. The intended way to figure out which nodes are changed (and query against the tree for what happened) is via TreeView.delta. I know it's an extra step, but you'll want to make a checkout for this and subscribe to the 'viewChange' event to do this.

Some sample code for the same pet project which does this is here.

I can also point you to a couple internal legacy shared-tree usages over teams if you want to look at those (but the diff logic is a lot more sprawling)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: durability: If I'm holding refs to two nodes A and B, and A updates - are both refs now "stale" or is my B ref still good?

What types are you holding onto exactly and how is one 'updating'?

Copy link
Contributor Author

@ChumpChief ChumpChief Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - I'll look into the checkout stuff then.

In this scenario if I have a node for nut quantity and a node for bolt quantity and am holding references to both - and then I update bolt quantity - I think the bolt quantity node is stale. But I'm unclear if the nut quantity node reference is still good or is also stale and needs to be refreshed from an updated view.

EDIT: I think these are TreeViewNode references, specifically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying data structure each of those nodes is part of is copy-on-write, so you can always query any node you have (more details on that structure here). You're right that the bolt quantity will be stale, but typically you'd refresh that by getting a checkout viewChange event, calling new.delta(old), seeing that the id for the bolt node is 'changed' (if you're representing the quantity as a payload on that node; if you do other edits maybe you'd see a delete/add for nested node if you had a boxed number, etc.), and realize you need to react in your application accordingly.

If you don't see a portion of the tree as added/deleted/changed while calling delta, and none of its ancestors have changed, it's safe to assume they are still valid and up-to-date content within the tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have this mostly working now - is there an equivalent in new SharedTree to find the changed nodes/delta (currently I'm just using "afterBatch")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see Craig's PR uses a "subtreeChanging" that sounds promising, I'll take a look at that.

Comment on lines 102 to 103
// REV: This seems to iterate in reverse order? Not sure why it swaps the entries as compared to how
// it was created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

traits are un-ordered. For some technical reasons that I forget the details of, we sort them by label in a few places. That might be what's going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - so if I wanted a stable display order, is there a best practice? I assume I'll need to add a node in the tree somewhere with some sortable characteristic.

Maybe I shouldn't be using part names for the traits, but instead have inventory->traits->parts[]->part->traits->name,quantity,sortableThing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few different ways to approach it depending on what you're modeling. You could create a trait which contains the 'order' for each other trait and only modify it as part of a transaction when adding/removing other traits entirely. This still has the problem of requiring 'fixup' when clients concurrently add two things with the same name, or when two clients concurrently remove the 2nd to last item (and thereby implicitly delete). For those reasons we generally don't advocate that approach.

I'll point out that you currently do have a stable display order, which is 'alphabetical'. It's just not append-only :). That's one approach which does work well for some problems: give the traits an inherent ordering by some means, and always display in that order.

If you want consistent display order which you have fine-grained control over, you could instead represent your inventory as a sequence of items, each having a type/quantity. Then you can insert items at exactly the point you please using normal sequence APIs. That's probably what I'd recommend here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this aligns with your thought. FWIW, I generally find YAML-style to be convenient expression of the domain model by giving a sample tree:

Inventory
  parts:
    - Part
        type: "nut",
        quantity: 5
    - Part
        type: "bolt"
        quantity: 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to have this structure - this incidentally caused the order to flip :-P

Re: using sequence APIs to control node order - do you have a pointer to an example of that? Would it be something like a series of Change.insertTree() calls per part when building the tree rather than putting them all in with a single BuildNode?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some of this section of the readme is a little out of date (I think the imperative API might've been removed?), but the section on StablePlace still looks good to me and is what you want: two concurrent edits which try to insert content "before the end of a trait" will end up with later-write-is-later-in-the-sequence, whereas the same edits which try to insert content "after the (current) last node in that trait" will end up with later-write-is-earlier-in-the-sequence.

That said, it doesn't look like you're modifying the sequence past initial insert, so it shouldn't really matter what you do here. What order are you getting in the current version of the code? is it not [nut, bolt]?

With the domain model suggested above, in particular I wouldn't expect this to be necessary:

  // REV: This is not a stable/controlled ordering - if we want to control the order, we might add
  // another node under the partNode that specifies some payload to order on.

that should just happen for free.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll try that out. I recognize that this is legacy shared tree so maybe not getting investment at the moment, but it is surprising to me that the provided order isn't preserved (basically implicitly doing this approach).

I am getting [nut, bolt] currently. My best guess is that the natural order has something to do with the specific nodeIds that are getting assigned, which changed when the tree structure changed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect [nut, bolt] in your current code because:


		const inventoryNode: BuildNode = {
			definition: "inventory",
			traits: {
				parts: [
					{
						definition: "part",
						traits: {
							name: {
								definition: "name",
								payload: "nut",
							},
							quantity: {
								definition: "quantity",
								payload: 0,
							},
						},
					},
					{
						definition: "part",
						traits: {
							name: {
								definition: "name",
								payload: "bolt",
							},
							quantity: {
								definition: "quantity",
								payload: 0,
							},
						},
					},
				],
			},
		};
		legacySharedTree.applyEdit(
			Change.insertTree(
				inventoryNode,
				StablePlace.atStartOf({
					parent: legacySharedTree.currentView.root,
					label: "inventory" as TraitLabel,
				}),
			),
		);

and the contents of the trait "parts" are ordered (which is consistent with the fact they're in an array)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha - so would it be fair to say "The enumeration order of traits on a node is stable but arbitrary, but the order of trait contents (if a sequence) is as-specified"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep.

});
}

// REV: Lots of casts and non-null assertions below - is there a more type-confident way to do this?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a little, but not too much: the lack of a schema system is one of the more painful things to deal with in legacy tree. The TreeNodeHandle API helps some with ergonomics of the API, but doesn't really fix the typing.

Simpler domain models might do well to write some boilerplate to hide the lower-level APIs (which is basically putting a poor-man's schema system on). That's what I did for a pet project written on legacy shared-tree for example, and it works OK but doesn't scale very well.

export type RootField = SchemaAware.TypedField<typeof rootField>;

export const schema = builder.intoDocumentSchema(rootField);
// REV: The rootField feels extra to me. Is there a way to omit it? Something like
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

current API design enables optional or sequence root fields, so supporting the existing API seems good.

You could probably make a reasonable case that builder.intoDocumentSchema(inventory) should be shorthand for builder.intoDocumentSchema(SchemaBuilder.field(FieldKinds.value, inventory)) though: it is likely the common case.


export type Inventory = SchemaAware.TypedNode<typeof inventory>;

export const schemaPolicy = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const schemaPolicy = {
export const schemaPolicy: InitializeAndSchematizeConfiguration<typeof rootField> = {

(with an import)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made this change in latest. Got me thinking about why this is merged schema + initialize though, added a REV comment for that.

}
}

// REV: One interesting challenge is that SharedTree and LegacySharedTree have the same Type ("SharedTree")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. We should just use a different "type" field for tree2 since we already have docs in the wild with "SharedTree" in the attributes blob. @taylorsw04 @CraigMacomber let's chat about this at standup on Monday (hopefully one of us will remember to bring it up :))

@ChumpChief
Copy link
Contributor Author

I'm going to pivot this into a separate demo per conversation here: #17609 (comment)

@ChumpChief
Copy link
Contributor Author

ChumpChief commented Oct 12, 2023

Closing this out, as it is now superseded by #17735

@ChumpChief ChumpChief closed this Oct 12, 2023
@ChumpChief ChumpChief deleted the TreeTutorial branch October 26, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants