Skip to content

docs(tree): merge semantics docs for each node kind#23040

Merged
yann-achard-MS merged 10 commits into
microsoft:mainfrom
yann-achard-MS:per-node-merge-semantics-docs
Nov 13, 2024
Merged

docs(tree): merge semantics docs for each node kind#23040
yann-achard-MS merged 10 commits into
microsoft:mainfrom
yann-achard-MS:per-node-merge-semantics-docs

Conversation

@yann-achard-MS
Copy link
Copy Markdown
Contributor

Description

Adds documentation describing the merge semantics of operations on each node kind in the tree.

Breaking Changes

None

@github-actions github-actions Bot added base: main PRs targeted against main branch area: dds Issues related to distributed data structures area: dds: tree labels Nov 8, 2024
@yann-achard-MS yann-achard-MS marked this pull request as draft November 8, 2024 22:28
Copy link
Copy Markdown
Collaborator

@msfluid-bot msfluid-bot left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

No packages impacted by the change.


Baseline commit: c7730cc
Baseline build: 306006
Happy Coding!!

Code coverage comparison check passed!!

@msfluid-bot
Copy link
Copy Markdown
Collaborator

msfluid-bot commented Nov 8, 2024

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 464.33 KB 464.36 KB +35 Bytes
azureClient.js 563.18 KB 563.23 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 262.48 KB 262.5 KB +14 Bytes
fluidFramework.js 426.84 KB 426.85 KB +14 Bytes
loader.js 134.18 KB 134.19 KB +14 Bytes
map.js 42.71 KB 42.71 KB +7 Bytes
matrix.js 148.36 KB 148.36 KB +7 Bytes
odspClient.js 528.97 KB 529.02 KB +49 Bytes
odspDriver.js 97.88 KB 97.9 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.83 KB +14 Bytes
sharedString.js 164.26 KB 164.27 KB +7 Bytes
sharedTree.js 417.3 KB 417.3 KB +7 Bytes
Total Size 3.37 MB 3.37 MB +245 Bytes

Baseline commit: c7730cc

Generated by 🚫 dangerJS against 97ada18

@yann-achard-MS yann-achard-MS marked this pull request as ready for review November 9, 2024 00:50
@yann-achard-MS yann-achard-MS requested a review from a team as a code owner November 11, 2024 16:50
Copy link
Copy Markdown
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

Mostly minor notes, a few bits of more relevant feedback. Also, I think these docs really should find their way to fluidframework.com. cc @Josmithr @rohandubal

Comment thread packages/dds/tree/docs/user-facing/array-merge-semantics.md Outdated
Comment thread packages/dds/tree/docs/user-facing/array-merge-semantics.md Outdated
Comment on lines +67 to +68
When that's the case,
the items end up ordered such that the items inserted by the edit that is sequenced earlier will appear after the items inserted by the edits that is sequenced later.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit, the first line is shorter than it could be and the second one longer. (I'm assuming you split them based on known conventions like 80 char lines, or IDE rulers; that's what I normally do).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, I'm splitting based on gramatically independent clauses (whith the hope of making it easier to do line-by-line review).
For example I can't split that long line into a standalone clause.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting, I hadn't seen that approach before, but I can see the appeal.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do the same FWIW. I split on sentences, and if I have a very long sentence then I'll split over a comma, and if there are no commas then I'll do what Yann did. Though, I'm starting to let my lines be longer and longer these days. Both viewers and editors have line wrap if you want them, so often the only practical advantage to splitting up the lines is to reduce merge conflicts, and that just doesn't happen that often with doc files.

Copy link
Copy Markdown
Contributor

@Josmithr Josmithr Nov 11, 2024

Choose a reason for hiding this comment

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

Breaking on sentence boundaries is documented as a best practice in our github wiki.
https://github.com/microsoft/FluidFramework/wiki/Markdown-Best-Practices#line-breaks-along-sentence-boundaries

Comment thread packages/dds/tree/docs/user-facing/array-merge-semantics.md Outdated
Comment thread packages/dds/tree/docs/user-facing/array-merge-semantics.md Outdated
Comment thread packages/dds/tree/docs/user-facing/map-merge-semantics.md Outdated
Comment thread packages/dds/tree/docs/user-facing/map-merge-semantics.md Outdated
Comment thread packages/dds/tree/docs/user-facing/map-merge-semantics.md
Comment on lines +67 to +69
Since Bob's client has yet to receive his own edit back from the sequencing service,
Bob's client can deduce that his edit is sequenced later and therefore wins out over Alice's.
This means the color property can remain blue.<br />
Copy link
Copy Markdown
Contributor

@alexvy86 alexvy86 Nov 11, 2024

Choose a reason for hiding this comment

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

Maybe add a footnote or something to indicate what would happen if Bob's operation is NACKed? The color would go back to red, I... think/hope? 😄 .

Also, do we need the <br/> here and a few lines above?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I don't think we support that in SharedTree. Once you've allowed an edit to be sent for sequencing, we forever re-try until you close the session.
I agree that may not always be ideal.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just realized Github helpfully [cough] rendered the <br/>, so I think the question stands, do we actually need them in md files?

Comment thread packages/dds/tree/docs/user-facing/object-merge-semantics.md Outdated
@microsoft microsoft deleted a comment from github-actions Bot Nov 11, 2024
@Josmithr
Copy link
Copy Markdown
Contributor

For the image files, if possible, we should avoid adding them directly to the repo. Alternative option: https://github.com/microsoft/FluidFramework/wiki/Uploading-images-for-the-website-to-Azure-blob-storage

@microsoft microsoft deleted a comment from github-actions Bot Nov 12, 2024
@yann-achard-MS
Copy link
Copy Markdown
Contributor Author

For the image files, if possible, we should avoid adding them directly to the repo. Alternative option: https://github.com/microsoft/FluidFramework/wiki/Uploading-images-for-the-website-to-Azure-blob-storage

@Josmithr - is that the recommended approach even for docs that are not part of the website? It seems that the only docs that currently use this approach are from the root docs folder.

@Josmithr
Copy link
Copy Markdown
Contributor

Josmithr commented Nov 12, 2024

For the image files, if possible, we should avoid adding them directly to the repo. Alternative option: https://github.com/microsoft/FluidFramework/wiki/Uploading-images-for-the-website-to-Azure-blob-storage

@Josmithr - is that the recommended approach even for docs that are not part of the website? It seems that the only docs that currently use this approach are from the root docs folder.

It is, though we need to do a better job of documenting that (and why). I have an item on my plate for unifying some of the documentation around that. One of the primary objectives is to reduce the number of binary files we check in to the repo.

@github-actions
Copy link
Copy Markdown
Contributor

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> fluidframework-docs@0.25.0 ci:linkcheck /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test ci:start 1313 linkcheck:full

1: starting server using command "npm run ci:start"
and when url "[ 'http://127.0.0.1:1313' ]" is responding with HTTP status code 200
running tests using command "npm run linkcheck:full"


> fluidframework-docs@0.25.0 ci:start
> http-server ./public --port 1313 --silent


> fluidframework-docs@0.25.0 linkcheck:full
> npm run linkcheck:fast -- --external


> fluidframework-docs@0.25.0 linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  443418 links
    3412 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


Copy link
Copy Markdown
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

LGTM!

@yann-achard-MS yann-achard-MS merged commit c7d9c18 into microsoft:main Nov 13, 2024
@yann-achard-MS yann-achard-MS deleted the per-node-merge-semantics-docs branch November 18, 2024 17:30
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.

5 participants