Skip to content
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

(tree): Add explicit revision tag to all changes #22525

Merged
merged 19 commits into from
Oct 1, 2024

Conversation

agarwal-navin
Copy link
Contributor

@agarwal-navin agarwal-navin commented Sep 16, 2024

Description

When the default edit builder generates a change, it does not have a revision tag associated with it. The revision tag is added later by calling ChangeRebaser.changeRevision. This PR updates these changes to have revision tag associated with it.
It also updates the invert function to generate changes with revision tags.

AB#13508

@agarwal-navin agarwal-navin requested a review from a team as a code owner September 16, 2024 18:57
@github-actions github-actions bot added base: main PRs targeted against main branch area: contributor experience area: dds Issues related to distributed data structures area: dds: tree labels Sep 16, 2024
.vscode/launch.json Outdated Show resolved Hide resolved
@@ -672,18 +673,19 @@ export class ModularChangeFamily
change.change.destroys === undefined,
0x89a /* Unexpected destroys in change to invert */,
);
const { revInfos } = getRevInfoFromTaggedChanges([change]);

if (hasConflicts(change.change)) {
return makeModularChangeset(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree its the current code is ugly and unclear. I think the best way to improve it would be to refator makeModularChangeset so that it takes a bag of named properies (most of which would be optional).

Copy link
Contributor Author

@agarwal-navin agarwal-navin Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I will do it in a separate PR. This PR is anyway too big :-). Created AB#15937 to do this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments are still here. Unresolving.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean. Do you want me remove the comments for params?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry that was not clear. Yes, that's what I had in mind. It's adding noise to this PR and the comments are likely to go stale without anyone noticing (or be wrong from the start as was the case at one point, I haven't checked the rest).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on this already. So, we can leave the comments :-). I will send out a PR soon.

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Sep 25, 2024

@fluid-example/bundle-size-tests: +2.3 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 463.2 KB 463.23 KB +35 Bytes
azureClient.js 558.69 KB 558.74 KB +49 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 261.33 KB 261.35 KB +14 Bytes
fluidFramework.js 402.51 KB 403.55 KB +1.04 KB
loader.js 134.17 KB 134.19 KB +14 Bytes
map.js 42.43 KB 42.44 KB +7 Bytes
matrix.js 148.83 KB 148.84 KB +7 Bytes
odspClient.js 525.84 KB 525.89 KB +49 Bytes
odspDriver.js 97.8 KB 97.82 KB +21 Bytes
odspPrefetchSnapshot.js 42.76 KB 42.78 KB +14 Bytes
sharedString.js 165.91 KB 165.91 KB +7 Bytes
sharedTree.js 392.97 KB 394.01 KB +1.04 KB
Total Size 3.31 MB 3.31 MB +2.3 KB

Baseline commit: ecdf26a

Generated by 🚫 dangerJS against f752971

@agarwal-navin agarwal-navin merged commit 7b7e736 into microsoft:main Oct 1, 2024
40 checks passed
@agarwal-navin agarwal-navin deleted the noImplicitRevision branch October 1, 2024 21:25
agarwal-navin added a commit that referenced this pull request Oct 1, 2024
…property bag (#22699)

From
#22525 (comment).

Updated `makeModularChangeset`, `buildModularChangesetFromField` and
`buildModularChangesetFromNode` to take a property bag as parameter.
These functions take a lot of parameters and it's currently a little
hard to tell them apart from calling them. Having a property bag makes
the parameters clear.
sonalideshpandemsft pushed a commit that referenced this pull request Oct 7, 2024
## Description
When the default edit builder generates a change, it does not have a
revision tag associated with it. The revision tag is added later by
calling `ChangeRebaser.changeRevision`. This PR updates these changes to
have revision tag associated with it.
It also updates the `invert` function to generate changes with revision
tags.


[AB#13508](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/13508)

---------

Co-authored-by: Yann Achard <[email protected]>
sonalideshpandemsft pushed a commit that referenced this pull request Oct 7, 2024
…property bag (#22699)

From
#22525 (comment).

Updated `makeModularChangeset`, `buildModularChangesetFromField` and
`buildModularChangesetFromNode` to take a property bag as parameter.
These functions take a lot of parameters and it's currently a little
hard to tell them apart from calling them. Having a property bag makes
the parameters clear.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: tree area: dds Issues related to distributed data structures base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants