WIP - Specific APIs for nodeChanged event depending on node-kind #7
Conversation
| * This callback should be called only once. | ||
| */ | ||
| on<K extends keyof ObjectNodeChangeEvents>( | ||
| node: TreeObjectNode, |
There was a problem hiding this comment.
My main problem in this PR: TreeObjectNode requires generic type parameters but I'm having a hard time figuring out what should I pass.
There was a problem hiding this comment.
If you don't know anything about what fields a node has, all you can say about an object node is thats its API is the same as TreeNode, but it might vane some more fields. There is no way to capture that in the type system that doesn't also allow all non-object nodes. There is nothing that can be used to restrict an API to only accepting object nodes, or to derive if a node is an object node from its type.
You can make an API that requires you to provide an object node schema, and a matching node since object node schema can be distinguished by their NodeKind. Thats the best you could do here, and I assume thats not at all what you want.
If you want it to be possible to write an APi that accepts any object node, we would have to add some property thats unique to object nodes. For example we could add a NodeKind symbol to tree node, an statically type it as NodeKind.Object for Object nodes. Without a change like that to the public TreeNode or object node APS, I'm pretty sure waht you want is impossible.
| readonly changedProperties: ReadonlySet<string>; | ||
| } | ||
|
|
||
| export interface TreeChangeEventsBase { |
There was a problem hiding this comment.
TreeChangeEventsBase keeps the common events that look the same regardless of node kind.
| treeChanged(): void; | ||
| } | ||
|
|
||
| export interface TreeChangeEventsInternal extends TreeChangeEventsBase { |
There was a problem hiding this comment.
TreeChangeEventsInternal adds nodeChanged with the payload, so the code that emits the events can emit them with the payload. That code is agnostic to the node kind so splitting the interface for object/array/map didn't seem to make sense in this case.
| @@ -59,7 +122,7 @@ export type Unhydrated<T> = T; | |||
| * | |||
| * @sealed @public | |||
| */ | |||
| export interface TreeChangeEvents { | |||
| export interface TreeChangeEvents extends TreeChangeEventsBase { | |||
There was a problem hiding this comment.
TreeChangeEvents stlil handles undifferentiated TreeNode, providing no payload for changedProperties. Not sure if should have it for everything, but I think trying to put it there led me to other errors.
| } | ||
|
|
||
| export interface ObjectNodeChangeEvents { | ||
| /** |
There was a problem hiding this comment.
Don't know if there's a better way to reuse all this documentation that needs little tweaks in each case.
| * 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; |
There was a problem hiding this comment.
Array nodes don't get a payload.
| // @ts-expect-error The Tree.on() overload for nodeChanged event for an array node should not have a payload. | ||
| // It still exists, but it should be an empty set for now. |
There was a problem hiding this comment.
This test maybe just wants to disappear instead... But then I don't know how to maintain confidence that we're not exposing the payload by mistake on array nodes.
…g. (#22222) ## Description See changeset for details. This will be used for things like providing object node specific events with strong typing. This is aimed to provide the functionality attempted in alexvy86#7 to better support node kind specific data in #22043
Trying out how to add APIs for subscribing to nodeChanged that are specific to each node kind, with different listeners.