-
Notifications
You must be signed in to change notification settings - Fork 532
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
feat: tree index api prototype #22491
base: main
Are you sure you want to change the base?
Conversation
packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts
Outdated
Show resolved
Hide resolved
packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts
Outdated
Show resolved
Hide resolved
const nodes = this.nodes.get(key); | ||
if (nodes !== undefined) { | ||
// if the key already exists in the index, the anchor node is appended to its list of nodes | ||
this.nodes.set(key, [...nodes, anchorNode]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to clone the array rather than push to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the array is readonly for api purposes and gets super ugly (not even sure if it's completely feasible) to make a bunch of casts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we just need the output type (the thing we hand to getValue) to implement readonly array...which array does? can we not just type our internal type as arry, e.g. private readonly nodes = new Map<TKey, AnchorNode[]>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we filter the array (meaning allocate a new one) before handing it to getValue anyway, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah, you're right, I was just doing something stupid when I tried it the first time
packages/dds/tree/src/feature-libraries/indexing/anchorTreeIndex.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
// index any new trees that are created later | ||
forest.on("afterRootFieldCreated", (fieldKey) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this event guaranteed to fire for all content created in the tree, including modifications of value fields (e.g. user does foo.x = 5, and the x field is indexed)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not, I created a skipped test and added a todo to use a different event. Will add the proposal for it in the api doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, so are indexes not totally broken until that works? is there another forest event that is fired when that happens that we can subscribe to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@noencke is claiming that primitives are wrapped in nodes and so even value nodes start out detached and are moved in. this sounds vaguely familiar. have you tested this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, that's what I thought too but then I wrote that skipped test to make sure and it failed
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
@@ -27,6 +27,9 @@ export interface CommitMetadata { | |||
readonly kind: CommitKind; | |||
} | |||
|
|||
// @public (undocumented) | |||
export function createSimpleTreeIndex<TKey extends TreeValue, TValue, TSchema extends TreeNodeSchema>(context: FlexTreeContext, indexer: (schema: TSchema) => KeyFinder<TKey> | undefined, getValue: (nodes: TreeIndexNodes<NodeFromSchema<TSchema>>) => TValue, indexableSchema: readonly TSchema[]): SimpleTreeIndex<TKey, TValue>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume these will go away (public and beta)?
export function createIdentifierIndex(context: FlexTreeContext): IdentifierIndex; | ||
|
||
// @alpha | ||
export function createSimpleTreeIndex<TKey extends TreeValue, TValue>(context: FlexTreeContext, indexer: (schema: TreeNodeSchema) => KeyFinder<TKey> | undefined, getValue: (nodes: TreeIndexNodes<TreeNode>) => TValue): SimpleTreeIndex<TKey, TValue>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this in alpha in the current form. Having users provide a KeyFinder (and take the burden of determinism/inval) seems like a bug pit. I feel like a reduced scope form (maybe they provide simple tree schema type + a value field pair and we have a KeyFinder that knows how to handle those) should be exposed first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely can't expose it until the value field mutation inval issue is solved (if it exists at all)
return value; | ||
} | ||
|
||
describe.only("tree indexes", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should have a clear-box test that checks that nodes that are GC'd after being removed are deleted from the index (triggered by "afterDestroy")
|
||
this.anchors.set(anchorNode, anchor); | ||
// when the anchor node is destroyed, delete it from the index | ||
anchorNode.on("afterDestroy", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having talked to Noah, we think this might be better written as a delta visitor instead of event subscriptions
Description
TreeIndex
is an interface based off of ReadonlyMap that indexes can adhere toTreeNode
sTODO expand out this description if we want to use this for the API review but maybe it'd be better to just review off the doc?
Reviewer Guidance
The review process is outlined on this wiki page.