Skip to content

Conversation

jenn-le
Copy link
Contributor

@jenn-le jenn-le commented Aug 14, 2025

Description

  • adds a landing page for schema evolution topics in fluid docs with general information
  • adds page on types of schema changes and how upgrading them works
  • adds page on sample policies from the shared tree package docs while simplifying them
  • removes doc from shared tree package

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.

@Copilot Copilot AI review requested due to automatic review settings August 14, 2025 23:11
@github-actions github-actions bot added area: dds Issues related to distributed data structures area: dds: tree area: website base: main PRs targeted against main branch labels Aug 14, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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.

@jenn-le
Copy link
Contributor Author

jenn-le commented Aug 15, 2025

As per discussion with Yann, policy 2 described should be replaced with instructions on using allowUnknownOptionalFields. I'm also not convinced the samples policy has much value so I might remove it.

```typescript
// Monitor for remote schema changes
view.events.on("schemaChanged", () => {
if (!view.compatibility.canView) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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):
Copy link
Contributor

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
Copy link
Contributor

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?


### Removing Fields

Removing fields from node types breaks older clients:
Copy link
Contributor

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:

  1. 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.
  2. 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!
Copy link
Contributor

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.

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 area: website base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants