Compat: Add minVersionForCollab option to declarative model and deprecate CompatibilityMode#27212
Compat: Add minVersionForCollab option to declarative model and deprecate CompatibilityMode#27212scottn12 wants to merge 20 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (310 lines, 25 files), I've queued these reviewers:
Toggle the reviewer checkboxes above to adjust, then tick the box below to start:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Fluid declarative model and service client APIs to support specifying an explicit minVersionForCollab (now aligned with the newer cross-client compatibility policy), while deprecating the legacy CompatibilityMode abstraction.
Changes:
- Promote
MinimumVersionForCollabto a@publicAPI in@fluidframework/runtime-definitions. - Add optional
minVersionForCollab?: MinimumVersionForCollabparameters to Tinylicious/Azure client container create/load flows and plumb through to the declarative model runtime factory creation. - Deprecate
CompatibilityMode(andminVersionForCollabOverride) while keepingcompatibilityModerequired for now to select runtime defaults.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace links for @fluidframework/runtime-definitions where newly imported. |
| packages/service-clients/tinylicious-client/src/TinyliciousClient.ts | Adds minVersionForCollab param and forwards it into runtime factory creation. |
| packages/service-clients/tinylicious-client/package.json | Adds dependency on @fluidframework/runtime-definitions. |
| packages/service-clients/tinylicious-client/api-report/tinylicious-client.public.api.md | Reflects new optional minVersionForCollab API surface. |
| packages/service-clients/tinylicious-client/api-report/tinylicious-client.beta.api.md | Reflects new optional minVersionForCollab API surface. |
| packages/service-clients/tinylicious-client/api-report/tinylicious-client.alpha.api.md | Reflects new optional minVersionForCollab API surface. |
| packages/service-clients/end-to-end-tests/azure-client/src/test/AzureClientFactory.ts | Updates test factory typing to include optional minVersionForCollab. |
| packages/service-clients/azure-client/src/interfaces.ts | Extends internal factory hook typing to accept optional minVersionForCollab. |
| packages/service-clients/azure-client/src/AzureClient.ts | Adds minVersionForCollab param(s) and forwards into runtime factory creation. |
| packages/service-clients/azure-client/package.json | Adds dependency on @fluidframework/runtime-definitions. |
| packages/service-clients/azure-client/api-report/azure-client.public.api.md | Reflects new optional minVersionForCollab API surface and deprecates CompatibilityMode export. |
| packages/service-clients/azure-client/api-report/azure-client.legacy.public.api.md | Same as public API report for legacy build. |
| packages/service-clients/azure-client/api-report/azure-client.legacy.beta.api.md | Same as public API report for legacy beta build. |
| packages/service-clients/azure-client/api-report/azure-client.beta.api.md | Same as public API report for beta build. |
| packages/runtime/runtime-definitions/src/compatibilityDefinitions.ts | Promotes MinimumVersionForCollab from @beta to @public. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.public.api.md | API report updated to include MinimumVersionForCollab as public. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.public.api.md | API report updated to include MinimumVersionForCollab as public. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.beta.api.md | Updates extracted tag for MinimumVersionForCollab to public. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md | Updates extracted tag for MinimumVersionForCollab to public. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.beta.api.md | Updates extracted tag for MinimumVersionForCollab to public. |
| packages/framework/fluid-static/src/utils.ts | Introduces helper to resolve min-version and runtime defaults; keeps compatibility-mode mapping. |
| packages/framework/fluid-static/src/types.ts | Marks CompatibilityMode deprecated in favor of minVersionForCollab. |
| packages/framework/fluid-static/src/treeRootDataObject.ts | Plumbs minVersionForCollab through declarative tree runtime factory creation. |
| packages/framework/fluid-static/src/rootDataObject.ts | Plumbs minVersionForCollab through declarative DO-provider runtime factory creation. |
| packages/framework/fluid-static/src/compatibilityConfiguration.ts | Adds deprecated-import lint suppression for now-deprecated CompatibilityMode. |
| packages/framework/fluid-static/api-report/fluid-static.public.api.md | Marks CompatibilityMode deprecated in extracted API. |
| packages/framework/fluid-static/api-report/fluid-static.legacy.public.api.md | Marks CompatibilityMode deprecated in extracted legacy API. |
| packages/framework/fluid-static/api-report/fluid-static.legacy.beta.api.md | Marks CompatibilityMode deprecated and reflects added minVersionForCollab param in legacy beta API. |
| packages/framework/fluid-static/api-report/fluid-static.beta.api.md | Marks CompatibilityMode deprecated in extracted beta API. |
| packages/framework/fluid-static/api-report/fluid-static.alpha.api.md | Marks CompatibilityMode deprecated in extracted alpha API. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Rather than keeping these two types separate, where practical in API surface have them as a single input.
|
@jason-ha, updated in latest to do this |
jason-ha
left a comment
There was a problem hiding this comment.
Would be nice to clean up more of the internal APIs but can be done later.
| minVersionForCollab: minVersionForCollabOverride, | ||
| minVersionForCollab: minVersionForCollabOverride ?? minVersionForCollab, |
There was a problem hiding this comment.
I think the preferred logic is:
- if compatibilityMode is semver (not "1" | "2"), then use it independent of minVersionForCollabOverride
- else minVersionForCollabOverride if defined
- else compatibilityMode promoted to semantic version
The subtle difference is that minVersionForCollabOverride really is ignored when new pattern is used.
There was a problem hiding this comment.
I removed minVersionForCollabOverride in latest so this should be simpler now
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
jason-ha
left a comment
There was a problem hiding this comment.
Looking good
You will need a second change set and 3.0 breaking issue under #23271 to deprecate the *Client methods.
There is a 3.0 issue for removing 1 from CompatibilityMode and I think that can transition to straight up CompatibilityMode removal.
| "rootDir": "../", | ||
| "outDir": "../../lib", |
There was a problem hiding this comment.
Ha! I just cleaned up to test packages that weren't following the standard pattern ( #27322 ).
Put gen-version is quite inflexible. So, this pattern makes sense.
Please just add a comment that this deviates to accommodate gen-version.
| import type { IMember } from "@fluidframework/fluid-static"; | ||
| import type { ISharedMap, IValueChanged } from "@fluidframework/map/legacy"; | ||
|
|
||
| export { pkgVersion } from "../packageVersion.js"; |
There was a problem hiding this comment.
I would love to have some explanation here.
gen-version doesn't make a lot of sense on its own for a test-only package. I think we want to note that it is used to get the current client version.
(A more [logically] ideal value would be something that is shared from FF package; though I don't think we would want to actually try to support that.)
It would also be awesome to rename the constant, which we can do from the export statement with pkgVersion as minVersionForCollab or pkgVersion as currentVersion or pkgVersion as latestVersion or ???
| "build:test": [ | ||
| "^api-extractor:esnext", | ||
| "^build:esnext", | ||
| "^tsc" | ||
| ] |
There was a problem hiding this comment.
This needs to have build:genver added.
Will conflict with #27265 but I expect this will merge first as that one is waiting on another PR to merge.
| }>; | ||
| public async createContainer<T extends ContainerSchema>( | ||
| containerSchema: T, | ||
| minVersionForCollab: MinimumVersionForCollab = "2.0.0", |
There was a problem hiding this comment.
Can OdspClient work with 1.x?
Instead of MinimumVersionForCollab might want to use 2.${bigint}.${bigint} or
MinimumVersionForCollab & `2.${string}` // which requires visit in 3.0
or
Exclude<MinimumVersionForCollab, `1.${string}`> // which allow MinV4C to float up to 3| * DDSes to use) will not be included here but rather on the FluidContainer class itself. | ||
| * | ||
| * Returned by {@link TinyliciousClient.createContainer} and {@link TinyliciousClient.getContainer} alongside the FluidContainer. | ||
| * Returned by `TinyliciousClient.createContainer` and `TinyliciousClient.getContainer` alongside the FluidContainer. |
There was a problem hiding this comment.
I think you want to use qualifiers to retain the link.
See https://deepwiki.com/microsoft/tsdoc/2.3-declaration-references#2-syntax-and-grammar
I think adding :(1) to what was there before should work.
| let view: TreeView<typeof Table>; | ||
|
|
||
| if (location.hash) { | ||
| ({ container } = await client.getContainer(location.hash.slice(1), containerSchema, "2")); |
There was a problem hiding this comment.
I don't trust our linting to catch all cases - here is one that was missed.
Highly recommend a test build with deprecated methods removed. That change can be kept as stash for when the break is made.
| --- | ||
| `createTreeContainerRuntimeFactory` no longer accepts `minVersionForCollabOverride` | ||
|
|
||
| The `minVersionForCollabOverride` property on the `props` argument of `createTreeContainerRuntimeFactory` has been removed. |
There was a problem hiding this comment.
That is a beta breaking change. It will need to be kept until 2.110 - file deprecation issue under #26499.
| }; | ||
| const odspClient = new OdspClient(clientProps); | ||
| const { container, services } = await odspClient.createContainer(containerSchema); | ||
| const { container, services } = await odspClient.createContainer(containerSchema, "2.0.0"); |
There was a problem hiding this comment.
Can we use a higher number? We don't want anyone to set 2.0.0 if they could be setting 2.100.0 - especially if onboarding.
| * semver string or a legacy `CompatibilityMode` value — into a precise | ||
| * `MinimumVersionForCollab`. | ||
| * | ||
| * TODO: This can be removed when the deprecated CompatibilityMode is removed - AB#73679 |
There was a problem hiding this comment.
nit: I think a preferred format is
| * TODO: This can be removed when the deprecated CompatibilityMode is removed - AB#73679 | |
| * TODO: AB#73679: This can be removed when the deprecated CompatibilityMode is removed |
| * In "1" mode we support full interop between 2.x clients and 1.x clients, | ||
| * while in "2" mode we only support interop between 2.x clients. | ||
| * | ||
| * @deprecated Specify the minimum Fluid Framework version directly via the |
There was a problem hiding this comment.
[No need to change] I think it would be okay to NOT deprecate CompatiblityMode because it isn't a type that customers use - it mostly adds noise our codebase making it look funny when we use it because we have to.
The deprecation is done via the method overrides.
| /** | ||
| * The CompatibilityMode selected determines the set of runtime options to use. In "1" mode we support | ||
| * full interop with true 1.x clients, while in "2" mode we only support interop with 2.x clients. | ||
| * The `minVersionForCollab` determines the set of runtime options to use. |
There was a problem hiding this comment.
Nit: I don't think this description is very accurate. Probably worth reworking this.
| * The CompatibilityMode selected determines the set of runtime options to use. In "1" mode we support | ||
| * full interop with true 1.x clients, while in "2" mode we only support interop with 2.x clients. | ||
| * The `minVersionForCollab` determines the set of runtime options to use. | ||
| * For a 1.x `minVersionForCollab` we support full interop with true 1.x clients. |
There was a problem hiding this comment.
Nit: these specifics probably belong in a @remarks block. The summary block should be reserved for a brief semantic description.
| /** | ||
| * See {@link CompatibilityMode} and compatibilityModeRuntimeOptions for more details. | ||
| * Minimum Fluid Framework version required for collaboration. Accepts a | ||
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string; |
There was a problem hiding this comment.
Nit
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string; | |
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} SemVer string; |
| * @param schema - The schema for the container | ||
| * @param compatibilityMode - Compatibility mode | ||
| * @param rootDataObjectFactory - A factory that can construct the root data object. | ||
| * @param config - Resolved minimum version for collab (required) and optional runtime option overrides. |
There was a problem hiding this comment.
Nit: It was a miss that we permitted the abbreviation "collab" in the type name for "MinVersionForCollab". Our guidelines generally call to avoid using abbreviations that aren't truly ubiquitous. "Min", for example, is well understood. "Collab" less so.
At the very least, I would request that we expand "collab" to "collaboration" in documentation and any contexts where we aren't referring to the "MinVersionForCollab" type directly.
| /** | ||
| * See {@link CompatibilityMode} and compatibilityModeRuntimeOptions for more details. | ||
| * Minimum Fluid Framework version required for collaboration. Accepts a | ||
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string; |
There was a problem hiding this comment.
Nit
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string; | |
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} SemVer string; |
| * | ||
| * @deprecated Specify the minimum Fluid Framework version directly via the | ||
| * `minVersionForCollab` parameter, which accepts a | ||
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string. The |
There was a problem hiding this comment.
Nit
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} semver string. The | |
| * {@link @fluidframework/runtime-definitions#MinimumVersionForCollab} SemVer string. The |
| /** | ||
| * Maps CompatibilityMode to a semver valid string that can be passed to the container runtime. | ||
| * Resolves the `compatibilityMode` input — either a `MinimumVersionForCollab` | ||
| * semver string or a legacy `CompatibilityMode` value — into a precise |
There was a problem hiding this comment.
Nit
| * semver string or a legacy `CompatibilityMode` value — into a precise | |
| * SemVer string or a legacy `CompatibilityMode` value — into a precise |
| * @param containerSchema - Container schema for the new container. | ||
| * @param compatibilityMode - Compatibility mode the container should run in. | ||
| * @param minVersionForCollab - Minimum Fluid Framework version required for collaboration, as a | ||
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). |
There was a problem hiding this comment.
Nit (PascalCase "SemVer", and omit the typing information, since that's already captured by the type-system)
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). | |
| * SemVer string (e.g. `"1.0.0"`, `"2.0.0"`). |
| /** | ||
| * Creates a new detached container instance in the Azure Fluid Relay. | ||
| * @typeparam TContainerSchema - Used to infer the the type of 'initialObjects' in the returned container. | ||
| * (normally not explicitly specified.) |
There was a problem hiding this comment.
Nit:
| * (normally not explicitly specified.) | |
| * This does not normally need to be specified explicitly. |
There was a problem hiding this comment.
(Same feedback for instances below)
| * @param containerSchema - Container schema for the new container. | ||
| * @param compatibilityMode - Legacy {@link @fluidframework/fluid-static#CompatibilityMode} value. | ||
| * @returns New detached container instance along with associated services. | ||
| * @deprecated Pass a `MinimumVersionForCollab` semver string (e.g. `"2.0.0"`) instead. The legacy |
There was a problem hiding this comment.
Nit
| * @deprecated Pass a `MinimumVersionForCollab` semver string (e.g. `"2.0.0"`) instead. The legacy | |
| * @deprecated Pass a `MinimumVersionForCollab` SemVer string (e.g. `"2.0.0"`) instead. The legacy |
| * @param containerSchema - Container schema used to access data objects in the container. | ||
| * @param compatibilityMode - Compatibility mode the container should run in. | ||
| * @param minVersionForCollab - Minimum framework version required for collaboration, as a | ||
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). |
There was a problem hiding this comment.
Nit
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). | |
| * SemVer string (e.g. `"1.0.0"`, `"2.0.0"`). |
| * @param version - Unique version of the source container in Azure Fluid Relay. | ||
| * @param compatibilityMode - Compatibility mode the container should run in. | ||
| * @param minVersionForCollab - Minimum framework version required for collaboration, as a | ||
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). |
There was a problem hiding this comment.
Nit
| * `MinimumVersionForCollab` semver string (e.g. `"1.0.0"`, `"2.0.0"`). | |
| * SemVer string (e.g. `"1.0.0"`, `"2.0.0"`). |
| * @param version - Unique version of the source container in Azure Fluid Relay. | ||
| * @param compatibilityMode - Legacy {@link @fluidframework/fluid-static#CompatibilityMode} value. | ||
| * @returns Loaded container instance at the specified version. | ||
| * @deprecated Pass a `MinimumVersionForCollab` semver string (e.g. `"2.0.0"`) instead. The legacy |
There was a problem hiding this comment.
Nit
| * @deprecated Pass a `MinimumVersionForCollab` semver string (e.g. `"2.0.0"`) instead. The legacy | |
| * @deprecated Pass a `MinimumVersionForCollab` SemVer string (e.g. `"2.0.0"`) instead. The legacy |
Description
This PR updates the declarative model to accept any (valid)
minVersionForCollab. This is intended to replace the existingCompatibilityModetype (deprecated in this PR), which accepts either"1"or"2". This was done to ensure the declarative model is aligned to support the latest changes to the cross-client compat policy (see #27064).The following are the major changes in this PR:
MinimumVersionForCollabpromoted from a beta to public API. This was done so it could be used in public declarative model APIs.CompatibilityModeparam increateDOProviderContainerRuntimeFactory()now also acceptsMinimumVersionForCollab(previously only theCompatibilityModetype).CompatibilityModetype (will be removed in 3.0).minVersionForCollabOverride(will be removed in 3.0).Reviewer Guidance
MinimumVersionForCollabwill require API council review. If it's easier, this can be split into a separate PR.Misc
AB#72040