Add Clone to SharedTree Revertible #23044
Conversation
github-actions
bot
added
base: main
jikim-msft
changed the title
There was a problem hiding this comment.
Code Coverage Summary
↑ packages.dds.tree.src.simple-tree.api:
Line Coverage Change: 0.01% Branch Coverage Change: No change
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 88.76% | 88.76% | → No change |
| Line Coverage | 82.35% | 82.36% | ↑ 0.01% |
↑ packages.dds.tree.src.shared-tree:
Line Coverage Change: 0.02% Branch Coverage Change: 0.15%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 90.88% | 91.03% | ↑ 0.15% |
| Line Coverage | 97.28% | 97.30% | ↑ 0.02% |
Baseline commit: aa2718b
Baseline build: 306963
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +919 Bytes
Baseline commit: afcf161 |
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no suggestions.
Files not reviewed (1)
- packages/dds/tree/src/core/index.ts: Evaluated as low risk
Comments skipped due to low confidence (2)
packages/dds/tree/src/shared-tree/treeCheckout.ts:634
- [nitpick] The method name 'clone' might be ambiguous. Consider renaming it to 'createClone' or 'duplicate' for better clarity.
clone: (forkedBranch?: TreeBranch) => {
packages/dds/tree/src/shared-tree/treeCheckout.ts:639
- [nitpick] The error message 'Unable to dispose a revertible that has already been disposed.' could be more descriptive. Consider providing more context about why this error might occur.
throw new UsageError("Unable to dispose a revertible that has already been disposed.");
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
github-actions
bot
added
the
area: framework
|
|
||
| it("clone revertible fails if trees are different", () => { | ||
| const viewA = asTreeViewAlpha(createLocalSharedTree("testSharedTreeOne")); | ||
| const viewB = asTreeViewAlpha(createLocalSharedTreeTwo("testSharedTreeTwo")); |
There was a problem hiding this comment.
For this test case, it's not important that the two trees have different schema - the part that matters is that they are different instances of a SharedTree. So, it should be perfectly fine to initialize viewB using a second call to createLocalSharedTree() rather than createLocalSharedTreeTwo(). And then you can delete createLocalSharedTreeTwo().
| @@ -453,6 +464,137 @@ describe("Undo and redo", () => { | |||
| revertible.revert(); | |||
| assert.equal(view.root.foo, 1); | |||
| }); | |||
|
|
|||
| it("reverts original & forked revertibles after making change to the original view", () => { | |||
There was a problem hiding this comment.
Can you write these tests within the loop that all of the other tests (except 1 - "can undo while detached") in this file are in? If you look at the describe block at the top of the file, you will see that it runs all the tests in "attached" and "detached" modes. If you adapt your tests here to use the getCheckout helper, it should be easy to have them do the same thing.
There was a problem hiding this comment.
Well, actually, it's a little weird. You have a couple of options.
- Write your tests in the pattern already established in this file. So, you don't define a schema, and instead you do what all the other tests do and use
createCheckoutandtree.editorto do edits. This isn't the best way to edit the tree, but it's consistent with how the rest of the file is doing it. So when somebody goes to convert this file later to the "better way", it won't really add any more work to change your tests since they'll all be updated in the same way. - Write your tests in a newer, better pattern - kind of what you are already doing, although I'd recommend you use something like
TestTreeProviderLiterather than writing the factory/runtime code yourself. This is nice because it uses a real schema, and the better APIs for editing. However, then we end up with two different patterns in this file - the existing ones, and your new ones. And somebody coming along later to fix this file will have to deal with both patterns.
My vote would be for (1). We should either convert this whole file to a better test pattern first, and then do this PR. Or, we should write new tests in the same way as the current pattern, and then convert them all at once.
Curious what you think. Either way, having your tests run against both "attached" and "detached" scenarios is valuable - and doing that and also (2) will mean some duplicated/divergent code.
There was a problem hiding this comment.
I think I can go along with case (1) and I've noticed that there are skipped cases with // TODO: unskip once forking revertibles is supported comment.
I can un-skip these pre-written test cases.
There was a problem hiding this comment.
I've tried to use createCheckout() and tree.editor but realized clone: (branch?: TreeBranch), thus I can't (and shouldn't) pass TreeCheckout. Unless there is a way to convert TreeCheckout to TreeBranch prior to calling clone(), I think I should rewrite the skipped tests with my test design.
What do you think?
There was a problem hiding this comment.
Just update createCheckout() to also return the SharedTree so that you can make a view from it via viewWith(). You can change createCheckout() however you want, really - since as we discussed above, we don't particularly like how any of this is currently operating.
There was a problem hiding this comment.
If it were me, I'd probably:
- Have
createTree()which does pretty much everything that the currentcreateCheckout()does, except that it returns a SharedTree - Change
createCheckout()to just doreturn createTree().checkout
| /** | ||
| * TODO: Add description | ||
| * | ||
| * @param forkedBranch - A target branch to apply the revertible to. If not given, spawns a cloned revertible of the original branch. |
There was a problem hiding this comment.
should prob add additional details about what we exactly expect of this branch, mainly that it has the same commit that the revertible is meant to revert. also, what happens if we have a cloned revertible of the original branch? are the lifetimes of the clone and the original tied together?
| revertible.dispose(); | ||
| } | ||
| }, | ||
| clone: (forkedBranch?: TreeBranch) => { |
There was a problem hiding this comment.
what was the reasoning behind allowing forkedBranch to be optional? why not just make it required and not allow clone to be called without a fork? the behavior for cloning a revertible onto the same checkout doesn't seem obvious to me and i'm not sure why we'd want to do so
There was a problem hiding this comment.
Cloning a revertible onto the same branch is useful for an application that wants to provide ownership of a revertible to multiple components. Suppose I have two components in my application that:
- Don't know about each other
- Are operating on the same branch (e.g. the main branch)
- Both want to be able to revert (e.g. they both have undo buttons)
You'd want to clone the revertible, giving one to the first component and the clone to the second. Or, perhaps the parent component retains ownership of the original revertible and gives a new clone to each subcomponent. If you can't clone the revertible for the same branch, then you have to share the same revertible between the parent and both components, and now they do need to know about each other so that they know when to (or when not to) dispose the revertible.
Regarding the parameter being optional, it's just a convenience. For an application like the above, which operates solely on the main branch, it would be annoying at best and confusing at worst to be forced to pass in the main branch every time it wants to do a clone. With it being optional, the user doesn't need to understand the branching feature at all in order to clone revertibles.
There was a problem hiding this comment.
I agree that it would be useful, but I'm not entirely convinced that the changes here sufficiently support it. At the very least, I think it needs to be documented better.
some thoughts:
- if one of the components does a revert, how does it get communicated to the other? do we just fail whenever the other ones tries to do a revert and we can't find the repair data anymore?
- it seems like the status is currently cloned as well, which is fine since revertibles can be disposed without being reverted, but I think we'd need some way to properly communicate between clones whether the repair data they share can still be used
- do we maybe want a different api for cloning with the same branch? it kinda sounds like the meaning changes depending on if we're cloning with a different branch or the same branch i.e. with a different branch, nothing you do with the cloned revertible will affect the original but with the same branch, they still have the same dependencies even though they can be owned by different components and disposed separately as well although i guess it would be somewhat similar to the case where a user explicitly does not dispose after revert and we call revert again on the same revertible and it just noops. which I still think it kinda strange.
- I think having the parameter be optional is fine if the behavior around operating on the same branch is more clear
There was a problem hiding this comment.
For a set of clones of the same revertible associated with the same branch, the repair data will not be collected until all clones have called dispose(). This is the intended behavior, and I believe is the currently behavior, but if it's not covered by tests then we should add more tests.
So, yes, if component A and component B each have a clone of a revertible, and are on the same branch, then when A does a revert it will do something, and when B does a revert it will (probably) no-op. This is what I would expect - the reason you cloned in the first place is so that A and B don't have to know what each other are doing, therefore, if B does an undo but it was already undone by A, nothing will happen (because it "already happened"). The other option which you alluded to in your first bullet is for B's undo stack to be changed along with A's undo stack. But if that's what the user wants, then A and B are not isolated and the user shouldn't have done a clone in the first place. Undo-stacks are probably 1:1 with branches most of the time, is my guess. Being able to clone a revertible on the same branch is more of a ref-counting thing. Like you have some code in your application that wants to ensure it can revert some specific data at some point in the future, so it holds on to a particular revertible - regardless of whether or not a clone of that revertible might get reverted anyway by some other means. I think it's likely to be a pretty uncommon scenario, but it might be useful to have as an option, it's easy for us to implement, and it's the natural behavior (IMO) of a parameterless call to clone.
A review of potential scenarios (e.g. with Nick) to inform the documentation would probably not be a bad idea. If we find that it's just too hard to explain why you would do this, then that could be a sign that we should just leave it out.
Co-authored-by: Jenn <[email protected]>
Co-authored-by: Jenn <[email protected]>
Co-authored-by: Jenn <[email protected]>
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Description
13864
This PR adds forkable revertible feature to the
Revertibleobject of SharedTree.DisposableRevertibleand replaced byRevertibleAlpha.clone()method to the new interface.TreeBranch(which is subset ofTreeCheckout) to access data necessary for revert operation.