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 alpha tree configuration APIs #22701

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

CraigMacomber
Copy link
Contributor

Description

See changeset for details.

Based on changes from #22566

Reviewer Guidance

The review process is outlined on this wiki page.

@CraigMacomber CraigMacomber requested review from a team as code owners October 1, 2024 22:25
@github-actions github-actions bot added area: contributor experience area: dds Issues related to distributed data structures area: dds: tree area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct changeset-present public api change Changes to a public API base: main PRs targeted against main branch labels Oct 1, 2024
@@ -149,6 +152,27 @@ function getCodecVersions(formatVersion: number): ExplicitCodecVersions {
return versions;
}

export function buildConfiguredForest(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This refactor, and splitting out ForestOptions do not strictly have to be part of this change, but are required for the intended use of some of these APIs in #22566 , and seems small and related enough to include here.

@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Oct 1, 2024

@fluid-example/bundle-size-tests: +441 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 460.47 KB 460.5 KB +35 Bytes
azureClient.js 557.45 KB 557.5 KB +49 Bytes
connectionState.js 724 Bytes 724 Bytes No change
containerRuntime.js 259.74 KB 259.76 KB +14 Bytes
fluidFramework.js 405.32 KB 405.43 KB +112 Bytes
loader.js 134.34 KB 134.36 KB +14 Bytes
map.js 42.46 KB 42.46 KB +7 Bytes
matrix.js 148.63 KB 148.64 KB +7 Bytes
odspClient.js 524.41 KB 524.46 KB +49 Bytes
odspDriver.js 97.84 KB 97.86 KB +21 Bytes
odspPrefetchSnapshot.js 42.81 KB 42.82 KB +14 Bytes
sharedString.js 164.82 KB 164.83 KB +7 Bytes
sharedTree.js 395.78 KB 395.88 KB +105 Bytes
Total Size 3.31 MB 3.31 MB +441 Bytes

Baseline commit: 0dc54b2

Generated by 🚫 dangerJS against 53d1395

}

// @alpha
export const SharedTreeFormatVersion: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we default to V3 (thus dubbing it our actual "V1") and that we do not support documents in V1 (and maybe V2?), how would you feel about only exposing V3?

Copy link
Contributor

Choose a reason for hiding this comment

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

My instinct is this is a foot-gun, otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I think we should just expose a single framework wide enum which lists every framework minor version which had an opt in compat impacting change, and its value of v2.0 would map to v3 in our internal format numbers.

Fixing that is mostly orthogonal to this change.

That said, if someone has legacy documents (from before 2.0), and wants to generate test documents so they can ensure their old production documents will continue to work, these extra option might be useful to them.

Since this is just alpha, I think we can sort out these details later, but I do agree this isn't want we want long term and/or when this goes public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that we haven't done the work needed to create separate public vs internal configuration, but as long as these are targeted at alpha / debugging usage I think it may be better to keep using the internal types here rather than adding all the complexity needed to create distinct public API configuration, at least for now.

I have added a TODO to the APi's private remarks. I'll let API reviewers decide if they want to block exposing these as alpha on that work.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you set this to V1? Do we support writing legacy documents (which are immediately broken/unsupported)? I thought we only support writing N and N-1.
I'll agree that allowing V2 writing seems fine for the (possibly nonexistent) customers that want to test support for that, but exposing V1 at all seems very strange to me. @noencke thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

(regardless, I did approve and don't think this needs to block this since it's alpha)

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't support V1 then I don't see why we'd expose V1, but exposing V2 makes sense for now, particularly since this is alpha.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we leave some of the context from this discussion in @privateRemarks in the code for future reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we leave some of the context from this discussion in @privateRemarks in the code for future reference?

Is there something more you want beyond what's already in fe8daec ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, no, I didn't see that. Looks good to me!

@@ -793,6 +821,12 @@ export class SchemaFactory<out TScope extends string | undefined = string | unde
readonly string: TreeNodeSchema<"com.fluidframework.leaf.string", NodeKind.Leaf, string, string>;
}

// @alpha
export interface SchemaValidationFunction<Schema extends TSchema> {
// (undocumented)
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has a @returns doc comment. I guess that doesn't work well with our tooling so I'll make it a "Returns" instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -149,6 +152,27 @@ function getCodecVersions(formatVersion: number): ExplicitCodecVersions {
return versions;
}

export function buildConfiguredForest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

github-actions bot commented Oct 3, 2024

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output


> [email protected] 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"


> [email protected] ci:start
> http-server ./public --port 1313 --silent


> [email protected] linkcheck:full
> npm run linkcheck:fast -- --external


> [email protected] linkcheck:fast
> linkcheck http://localhost:1313 --skip-file skipped-urls.txt --external

Crawling...

Stats:
  421882 links
    3264 destination URLs
       2 URLs ignored
       0 warnings
       0 errors


@CraigMacomber CraigMacomber merged commit 40d3648 into microsoft:main Oct 3, 2024
33 checks passed
@CraigMacomber CraigMacomber deleted the AlphaConfig branch October 3, 2024 20:09
sonalideshpandemsft pushed a commit that referenced this pull request Oct 7, 2024
## Description

See changeset for details.

Based on changes from
#22566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: contributor experience area: dds: tree area: dds Issues related to distributed data structures area: framework Framework is a tag for issues involving the developer framework. Eg Aqueduct base: main PRs targeted against main branch changeset-present public api change Changes to a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants