Expose squash method on SharedTreeBranch #23170
Conversation
github-actions
bot
added
area: dds
| } | ||
|
|
||
| const removedCommits: GraphCommit<TChange>[] = []; | ||
| const ancestor = findAncestor([this.head, removedCommits], (c) => c === startCommit); |
There was a problem hiding this comment.
This is, for now, introducing duplicated work (because "removedCommits" is already stored by the transaction state). However, it is necessary to do this independently if we want to later remove transactions from branches, which is the goal. I don't expect this to actually regress any behavior, except (maybe?) in the case of an absolutely massive transaction.
There was a problem hiding this comment.
Doesn't bother me :)
There was a problem hiding this comment.
Code Coverage Summary
↑ packages.dds.tree.src.shared-tree-core:
Line Coverage Change: 0.03% Branch Coverage Change: 0.98%
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 92.59% | 93.57% | ↑ 0.98% |
| Line Coverage | 97.15% | 97.18% | ↑ 0.03% |
Baseline commit: 645a1a0
Baseline build: 309185
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +505 Bytes
Baseline commit: 645a1a0 |
| if (c !== commit) { | ||
| const revision = this.mintRevisionTag(); | ||
| const inverse = this.changeFamily.rebaser.changeRevision( | ||
| this.changeFamily.rebaser.invert(c, true, revision), | ||
| revision, | ||
| c.revision, | ||
| ); | ||
|
|
||
| inverses.push(tagRollbackInverse(inverse, revision, c.revision)); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I'm not a fan of having side effects within the findAncestor callstack. I don't think the contract for findAncestor should be so specific as to guarantee this will work as expected (it currently hints that way but I don't think it should). Seems like the legibility of the code would go up anyway if we pulled this out.
There was a problem hiding this comment.
Hmm... I like this way. If you pull it out, then you have to:
- Write an additional loop...
- ...which is a backwards loop, which means you can't use
for ofbut have to use indices... - ...which means you have to assert that the read in every iteration is not undefined...
You can see this in the equivalent code that was removed from abortTransaction. In terms of legibility and code niceness, I definitely prefer this version.
Is there a way we can strengthen the contract of findAncestor to your liking? It already says "predicate - a function which will be evaluated on every ancestor of descendant until it returns true." Are you concerned that it doesn't specify the order that it evaluates the ancestors, or something? We could change it to "...on each ancestor (in ascending order) of descendant...".
I'm generally a fan of writing "functional-style" code - i.e. chaining steps together sequentially rather than trying to do two things at once like is being done here. But that's because it usually makes the code prettier - in this case it makes it worse.
There was a problem hiding this comment.
Well, I guess the true "functional-style" way to do it would be to reverse() the array before iterating. But I don't think we want to do that for perf :)
Description
This exposes the ability to directly remove or squash commits on the head of a SharedTreeBranch. Currently this functionality is used when aborting or committing transactions. In addition to being a nice refactor in general, this will assist with future efforts to remove the notion of transactionality from branches.