-
Notifications
You must be signed in to change notification settings - Fork 554
docs(tree): general schema evolution docs #25218
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR consolidates and simplifies schema evolution documentation for SharedTree by moving docs from the package to the main Fluid docs site and making them more user-friendly.
- Removes complex package-specific documentation and consolidates into user-facing docs
- Creates structured schema evolution documentation with landing page, types of changes, and compatibility policies
- Adds cross-references between schema definition and evolution documentation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/dds/tree/docs/user-facing/schema-evolution.md | Removes detailed package-specific schema evolution documentation |
docs/docs/data-structures/tree/schema-evolution/index.mdx | Adds main schema evolution landing page with overview and upgrade process |
docs/docs/data-structures/tree/schema-evolution/types-of-changes.mdx | Creates dedicated page explaining safe vs breaking schema changes |
docs/docs/data-structures/tree/schema-evolution/compatibility-policies.mdx | Adds sample compatibility policies extracted from package docs |
docs/docs/data-structures/tree/schema-definition.mdx | Links to new schema evolution docs |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
docs/docs/data-structures/tree/schema-evolution/compatibility-policies.mdx
Outdated
Show resolved
Hide resolved
docs/docs/data-structures/tree/schema-evolution/compatibility-policies.mdx
Outdated
Show resolved
Hide resolved
docs/docs/data-structures/tree/schema-evolution/compatibility-policies.mdx
Outdated
Show resolved
Hide resolved
docs/docs/data-structures/tree/schema-evolution/compatibility-policies.mdx
Outdated
Show resolved
Hide resolved
…policies.mdx Co-authored-by: yann-achard-MS <[email protected]>
As per discussion with Yann, policy 2 described should be replaced with instructions on using |
Co-authored-by: yann-achard-MS <[email protected]>
…uidFramework into schema-evolution-docs
```typescript | ||
// Monitor for remote schema changes | ||
view.events.on("schemaChanged", () => { | ||
if (!view.compatibility.canView) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if the schema changes and canView
is falso but canUpgrade
is true? Is the comment below implying that if the user reloads, then the upgrade will be done upon reloading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on canUpgrade, I was thinking that shouldn't be possible but it def is. The comment below is confusing and unnecessary so I just removed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why mdx instead of md?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost all the pages in the folder are mdx so I decided to just copy that. It's probably not necessary and maybe everyone was thinking that and that's how we got to this point but I figured it doesn't hurt.
@Josmithr should we be keeping to using md files on the site?
|
||
### Making Required Fields Optional | ||
|
||
Converting a required field to optional is always [backwards compatible](./index.mdx#backwards-compatibility): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also forwards compatible? (It's not). The section below calls that out, but this section does not.
Maybe for each of these sentences in each section, replace with a little table
BC | FC
✅ | ❌
radius: factory.number, // Required | ||
}) {} | ||
|
||
// After - Safe change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we actually make such a change without a stagedOptional
API?
docs/docs/data-structures/tree/schema-evolution/types-of-changes.mdx
Outdated
Show resolved
Hide resolved
|
||
### Removing Fields | ||
|
||
Removing fields from node types breaks older clients: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably breaks new clients opening old documents too - this is not an approved case that upgradeSchema
will handle, is it?
Maybe we need to decide whether this document is describing:
- The possible behavior for an abstract system with the rules described so far. In that case, yes, removing fields necessarily breaks older clients, but whether it breaks new clients is a detail of the particular APIs.
- The specific behavior for SharedTree and its current APIs.
I think for a public facing document like this, we want to do 2. If we want more generalized theoretical documents like 1, for our own future ideation, they should probably be internal... or if we want to share them with the public, then a blog-post or technical paper kind of thing.
```typescript | ||
// Breaking: Removing allowed type | ||
class Whiteboard extends factory.object("Whiteboard", { | ||
elements: factory.array([TextElement]), // ImageElement removed - BREAKING! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps introduce a schema up above that is the baseline, and then each of these examples can show the schema after the hypothetical change. That way the user can consistently diff to see exactly what the change looks like.
Co-authored-by: Noah Encke <[email protected]>
Co-authored-by: Noah Encke <[email protected]>
Co-authored-by: Noah Encke <[email protected]>
Co-authored-by: Noah Encke <[email protected]>
…es.mdx Co-authored-by: Noah Encke <[email protected]>
Co-authored-by: Noah Encke <[email protected]>
Co-authored-by: Noah Encke <[email protected]>
…es.mdx Co-authored-by: Noah Encke <[email protected]>
Description
Reviewer Guidance
This consolidates the docs on schema evolution we had stored in a few different locations and attempts to simplify them for users of SharedTree, leaving out a lot of the more conceptual information on tools that could be implemented to make schema evolution better and just focusing on what can be used.
Some of the docs are existing and just copied from other places but everything should be reviewed in case we can improve them. In particular, the example code in the sample policies page should be reviewed.
Staged allowed types are only briefly mentioned but will get more comprehensive docs/linking in a separate pr.