EditableTree: transaction-based editing#12182
Conversation
github-actions
Bot
added
area: dds
| }); | ||
| }); | ||
|
|
||
| it("can edit using editable-tree", async () => { |
There was a problem hiding this comment.
Was removing this test intended? If so why? Seems like we could update it to use the new API.
There was a problem hiding this comment.
I think it was accidental, as my VS Code didn't list this change before I pushed. I'll put it back.
There was a problem hiding this comment.
@CraigMacomber Restored, but without anchorSymbol, which seems to be not needed anymore as an editing is implemented here, to some extent at least.
However, as more I think about that, more I get to the point, that it might actually be very useful to be able to get a path for a node of EditableTree. What do you think?
| "Cannot resolve a field type, use 'insertNodeSymbol' instead"); | ||
| const name = [...fieldSchema.types][0]; | ||
| const type: NamedTreeSchema = { name, ...target.context.forest.schema.lookupTreeSchema(name) }; | ||
| const jsonValue = isPrimitiveValue(value) ? value : value as object; |
There was a problem hiding this comment.
First I want to make sure I understand the editing contract that is implemented here: if I am following the code correctly, it looks like this adds support for three kinds of "set" operations:
- Overwriting the primitive value of an existing node.
- Inserting a new leaf node that represents the given primitive value where no such node exists currently.
- Inserting a new subtree where no such node exists currently.
Is that correct?
There was a problem hiding this comment.
Yep, that's correct more or less. Currently it's limited with a functionality of TypedJsonCursor, e.g., one shouldn't use it if a schema contains polymorphic fields (same for the context of a subtree), or optional fields, etc.. Still a lot to be done here.
There was a problem hiding this comment.
You could also, in the case where there is a subtree present for the given key and the given value is not a primitive, replace the existing subtree with a combination of delete and insert.
I'm not demanding that you add that in this PR though, just pointing out the possibility.
Feel free to close this comment.
There was a problem hiding this comment.
I like the idea. I'll keep in mind.
| assert.equal("zip" in person.address, true); | ||
| assert.throws(() => { | ||
| person.address[insertNodeSymbol]("zip", singleTextCursor({ value: 99038, type: int32Schema.name })); | ||
| }, /Insertion into a non-empty non-sequence field. Consider to use 'setValueSymbol' or delete the node first./); |
There was a problem hiding this comment.
It seems like setValueSymbol is not supported yet, but its exposed in the API
| const [provider, trees] = await createSharedTrees(fullSchemaData, personData, 2); | ||
| const person = trees[0].root as PersonType; | ||
| assert(isUnwrappedNode(person.address)); | ||
| assert(isEditableFieldSequence(person.address.phones)); |
There was a problem hiding this comment.
What happens if the property is not editable and you try to edit/delete it via the proxy, does it throw an exception ?
If yes, can we add a test case for that ?
There was a problem hiding this comment.
If it is not a sequence, you can read and change it as an object. If you mean the line above, in a negative case you get a primitive. Anyway, you can edit/delete it using its predecessor and symbols, for deletes even like delete person.address.phones.
| * @returns the number of immediate children for the given key of the currently selected node. | ||
| */ | ||
| length(key: FieldKey): number; | ||
| length(key?: FieldKey): number; |
There was a problem hiding this comment.
If you are going to change the API here, you need to update the documentation and tests accordingly.
If no key is provided, it looks like you are returning the number of siblings of the current node?
This seems like a reasonable change to make. You might also an extra feature in the new cursor API to handle whatever needed this change as well.
This PR provide a way to access fields of EditableTree without
unwrapping. Currently, this works only on "all or nothing" basis meaning
that (given we got an instance of EditableTreeContext with
`getEditableTreeContext()`):
- one can call `context.unwrappedRoot` to get root of EditableTree with
all children fields unwrapped into just values of their nodes if those
are primitives and the field has a non-sequence schema, nodes with a
sequence primary field ("arrays") unwrapped into arrays and so on.
- and one can call `context.root` to get root field typed as
`EditableField`, where no unwrapping happens, and then iterate over its
nodes, which is just an array of EditableTrees.
In turn, one can iterate over `EditableTree` to get its fields as
EditableFields.
Please, check the `"traverse a complete tree (generic)"` test case and
the corresponding `expectFieldEquals` and `expectNodeEquals` helper
functions to see how this works.
Opinions, comments, suggestions are very welcomed.
**Step 3** in a series to push code from #12182 to enable editing of
EditableTree.
[Step 2](#12306)
[Step 1](#12300)
Same as [#12488](#12512), but using a new cursor API. ## Description This PR implements `EditableField` as an array-like sequence of nodes (`EditableTrees`), which 1) replaces handling of sequence fields with arrays, 2) allows to access nodes lazily by their indices with and without unwrapping, and 3) is a required change to implement editing of `EditableTree`. ## Reviewer Guidance Everything around "root" probably might be considered as a workaround as currently `cursor.getPath()` and anchors do not support fields. Also, it seems that "primaryField" property of `FieldProxyTarget` is redundant, as `fieldKey` is also there, but it adds clarity, and we might make use of it when implementing editing. ## Other information or known dependencies **Step 4** in a series to push code from #12182 to enable editing of EditableTree. [Step 3](#12428) [Step 2](#12306) [Step 1](#12300) Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
) Same as [microsoft#12488](microsoft#12512), but using a new cursor API. ## Description This PR implements `EditableField` as an array-like sequence of nodes (`EditableTrees`), which 1) replaces handling of sequence fields with arrays, 2) allows to access nodes lazily by their indices with and without unwrapping, and 3) is a required change to implement editing of `EditableTree`. ## Reviewer Guidance Everything around "root" probably might be considered as a workaround as currently `cursor.getPath()` and anchors do not support fields. Also, it seems that "primaryField" property of `FieldProxyTarget` is redundant, as `fieldKey` is also there, but it adds clarity, and we might make use of it when implementing editing. ## Other information or known dependencies **Step 4** in a series to push code from microsoft#12182 to enable editing of EditableTree. [Step 3](microsoft#12428) [Step 2](microsoft#12306) [Step 1](microsoft#12300) Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
) Same as [microsoft#12488](microsoft#12512), but using a new cursor API. ## Description This PR implements `EditableField` as an array-like sequence of nodes (`EditableTrees`), which 1) replaces handling of sequence fields with arrays, 2) allows to access nodes lazily by their indices with and without unwrapping, and 3) is a required change to implement editing of `EditableTree`. ## Reviewer Guidance Everything around "root" probably might be considered as a workaround as currently `cursor.getPath()` and anchors do not support fields. Also, it seems that "primaryField" property of `FieldProxyTarget` is redundant, as `fieldKey` is also there, but it adds clarity, and we might make use of it when implementing editing. ## Other information or known dependencies **Step 4** in a series to push code from microsoft#12182 to enable editing of EditableTree. [Step 3](microsoft#12428) [Step 2](microsoft#12306) [Step 1](microsoft#12300) Co-authored-by: Craig Macomber (Microsoft) <42876482+CraigMacomber@users.noreply.github.com>
|
done |
6 participants
An initial implementation of a transaction-based editing with EditableTree.
Maybe it was too much to put both editing and handling sequences here.
Everything in this PR may be discussed, changed, removed.
In general, supported editing scenarios so far:
JS-like updates (
proxy.name = "adam") work only for primitive values. Same for inserts i.e. assigning the field which does not yet exist.If absolutely needed to change an array, one could remove it completely from the tree and insert a new one =)
Keep in mind that deleting a node with children will currently leave an "anchor trace" in a forest. This is how
AnchorSetworks right now.