Tweak support for 3.x in OldestSupportedClientVersion - #28022
Tweak support for 3.x in OldestSupportedClientVersion#28022Craig Macomber (Microsoft) (CraigMacomber) wants to merge 9 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (102 lines, 25 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
This PR refines how Fluid Framework v3 is represented in OldestSupportedClientVersion, simplifying v3 support by restricting it to major+minor (with patch 0) while keeping legacy flexibility for v1/v2, and updates tests and end-to-end test packages accordingly.
Changes:
- Tightens
OldestSupportedClientVersionso v3 is expressed as3.<minor>.0(minor-only), while retaining v1/v2 patch + prerelease forms. - Updates multiple tests to use
cleanedPackageVersion/featureVersion(pkgVersion)when supplying “current” version values. - Adds
@fluidframework/driver-definitionsdependency to service-client end-to-end test packages (and updates lockfile) to usefeatureVersion.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates workspace links to reflect added dependency usage. |
| packages/test/test-end-to-end-tests/src/test/containerRuntime.spec.ts | Uses cleanedPackageVersion for min-version telemetry assertions. |
| packages/service-clients/end-to-end-tests/odsp-client/src/test/containerCreate.spec.ts | Uses featureVersion(pkgVersion) for service-client version formatting. |
| packages/service-clients/end-to-end-tests/odsp-client/src/test/audience.spec.ts | Uses featureVersion(pkgVersion) for service-client version formatting. |
| packages/service-clients/end-to-end-tests/odsp-client/package.json | Adds @fluidframework/driver-definitions dependency. |
| packages/service-clients/end-to-end-tests/azure-client/src/test/utils.ts | Uses featureVersion(pkgVersion) and documents internal import. |
| packages/service-clients/end-to-end-tests/azure-client/package.json | Adds @fluidframework/driver-definitions dependency. |
| packages/runtime/runtime-utils/src/test/compatibilityBase.spec.ts | Aligns tests to validate against cleanedPackageVersion. |
| packages/runtime/runtime-definitions/src/compatibilityDefinitions.ts | Adjusts v3 version shape in OldestSupportedClientVersion and updates docs. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.public.api.md | Regenerated API report reflecting updated type. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.public.api.md | Regenerated API report reflecting updated type. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.beta.api.md | Regenerated API report reflecting updated type. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md | Regenerated API report reflecting updated type. |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.beta.api.md | Regenerated API report reflecting updated type. |
| packages/runtime/container-runtime/src/test/containerRuntime.spec.ts | Updates test naming/behavior around minVersionForCollab. |
| packages/dds/tree/src/test/shared-tree/fuzz/baseModel.ts | Uses cleanedPackageVersion for fuzz factory min-version configuration. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ntVersion3 # Conflicts: # packages/service-clients/end-to-end-tests/azure-client/src/test/utils.ts
| export type OldestSupportedClientVersion = | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}` | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}-${string}`; | ||
| | `3.${bigint}.0` |
There was a problem hiding this comment.
- Let's not force
.0. If we want to ignore patch and use3.${bigint}, then I am okay with that. - I would like to be convinced there is not case where it would be useful to limit the oldest to some patch. Perhaps it isn't meaningful to FF SDK directly but say there is some issue fixed in a patch and it could be useful on customer side to say they don't want to let anyone open document if they don't have the patch.
There was a problem hiding this comment.
This is a package version, so it needs to have all three digits: any change to that would be a major change, and I think makes it out of scope as something we can do now.
Specifying patches doesn't work as one would expect, since if you say 3.1.8, you are saying there might be clients whos version is as old as 3.1.8. Our versioning age logic however considers 3.2.0 newer than that, despite it possibility being older and not having the patch.
Remember that this controls opting into FF features. If you specify 3.1.8 you are saying it's ok to opt into features which are only supported by that patch release but not the prior patch. But you are also implicitly supporting 3.2.0 (because of how we sort versions) which might not have the patch.
We don't add features in patch releases, and if a given feature is bugged in some minor, not having OldestSupportedClientVersion opt into it until the next minor seems safe generally and reducing confusion and risk of hitting bugs.
Also note that letting people specify patch version in the future if we have some case where for some reason its desired would be a non-breaking change. At that point we would want to clarify the semantics around it.
There was a problem hiding this comment.
I am not convinced that it is a package version. In semver syntax I think it is the expression that completes ">=ver" spec.
I would not be surprised if using 3.{bigint} just worked.
There was a problem hiding this comment.
I think our docs phrase it as a package version at least as low as any version of any (non-loader) fluid framework client package that may be used on this document, though you are right we could rephrase that into something that compares less than or equal to any such package versions.
I suspect with some changes to the implementation, validation and normalization we could let 3.{bigint} work, but currently I don't think it does. Allowing that in the future would be a non breaking change though.
There was a problem hiding this comment.
I have updated the privateRemarks calling out the option of, as a non breaking change, allowing dropping the .0, but also calling out a potential risk of that.
There was a problem hiding this comment.
Assuming we keep the 3.minor.0 contract, I think the runtime validator needs to enforce the same rule. validateMinimumVersionForCollab currently checks only valid semver, the lower floor, and <= cleanedPackageVersion .
For example, once 3.2.0 ships, validateMinimumVersionForCollab("3.1.8") would still succeed even though this type rejects it.
There was a problem hiding this comment.
I played around last night trying out dropping patch. It looked good for a while but there is too much code that relies on comparing exact versions and major.minor is not a valid semantic version (is a range, but does not imply .0).
This is a big ugly, reveals complications trying to do it, and points out some changes we should have in this PR. The diff is here: https://github.com/CraigMacomber/FluidFramework/compare/OldestSupportedClientVersion3...microsoft:FluidFramework:OldestSupportedClientVersion3_without_patch?expand=1
I will try to comment on individual places to make feedback more digestible.
There was a problem hiding this comment.
I had trouble commenting elsewhere since related files aren't touched. One of the comments is what I see Willie noting. I have a version of validation at
FluidFramework/packages/runtime/runtime-utils/src/compatibilityBase.ts
Lines 224 to 240 in 89a9be2
| workloadName: "SharedTree (Reference Forest)", | ||
| factory: new SharedTreeFuzzTestFactory(createOnCreate(undefined), undefined, { | ||
| minVersionForCollab: pkgVersion, | ||
| minVersionForCollab: cleanedPackageVersion, |
There was a problem hiding this comment.
It doesn't seem great to get version from another package since it could be higher than this package's version. But since this is a test that probably works out.
Since cleanedPackageVersion is a bit dangerous, it would best to have a /test export/import path.
I see there is one production use in tree/codec with a good-sized disclaimer. Seems like that case is a candidate to use base version (major.minor.0 or major.minor).
There was a problem hiding this comment.
I feel like pkgVersion is the dangerous thing.
Not using the cleaned version can cause a feature that you think you are testing (enabled in the current version) to not be tested on CI, since CI changes the pkgVersion to be lower semver wise (by adding a prerelase tag) before running the test, and thus skips testing the new feature which this code claimed to be testing and will be enabled in the next releases.
There was a problem hiding this comment.
I'm not saying pkgVersion isn't dangerous. Just that ~ dep on runtime-utils technically means it could be more advanced that current package's version.
Building cleaned version from pkgVersion seems like the way to go. We could even generate the clean version into packageVersion.ts.
There was a problem hiding this comment.
I have made this use the existing package local currentVersion (which already documents this issue) so this PR doesn't regress this.
I like the idea of including the cleaned/feature version in generated packageVersion.ts as a longer term solution though.
There was a problem hiding this comment.
Might be misunderstanding, but with this PR’s new type, a 3.1.2 build would produce currentVersion === "3.1.2", even though OldestSupportedClientVersion now accepts only 3.minor.0 ?
…to OldestSupportedClientVersion3
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See Checking for Broken Links for more information. linkcheck output |
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
Joshua Smithrud (Josmithr)
left a comment
There was a problem hiding this comment.
API changes look good, and seem aligned with the outcomes of the PR discussions.
But I did not review the code changes in detail. It would be good to get Jason and/or Willie to sign off as well, since they've been doing work in this space.
Docs question: are there any existing changesets around oldestSupportedClient that should be updated to reflect the new requirements?
This is adjusting the work from #28003 which had not changeset, so I don't think there is one yet. I think it might make sense to leave that for #27972 which also adjusts this, and is the actual breaking change for 3.0 here |
Jason Hartman (jason-ha)
left a comment
There was a problem hiding this comment.
I'm now good with .0 but we need at least some validation changes.
| "@types/sinon": "^17.0.3", | ||
| "c8": "^10.1.3", | ||
| "eslint": "catalog:eslint", | ||
| "fluid-framework": "workspace:~", |
There was a problem hiding this comment.
How confident are you that a dev dep is okay for this test package?
I have a niggling suspicion that this needs to be a prod dep.
| export type OldestSupportedClientVersion = | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}` | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}-${string}`; | ||
| | `3.${bigint}.0` |
There was a problem hiding this comment.
I played around last night trying out dropping patch. It looked good for a while but there is too much code that relies on comparing exact versions and major.minor is not a valid semantic version (is a range, but does not imply .0).
This is a big ugly, reveals complications trying to do it, and points out some changes we should have in this PR. The diff is here: https://github.com/CraigMacomber/FluidFramework/compare/OldestSupportedClientVersion3...microsoft:FluidFramework:OldestSupportedClientVersion3_without_patch?expand=1
I will try to comment on individual places to make feedback more digestible.
| } from "@fluidframework/driver-definitions/internal"; | ||
| export { | ||
| createBasicRegistryKey, | ||
| featureVersion, |
There was a problem hiding this comment.
Yes, it is alpha, but exposing this seems like it will make it just a bit harder to lop off the .0 in the future if we want to.
Also am not a fan of the name FWIW. Is there no technical term for this? I've used "base" version for this in some cases. Just to spit ball two names "patchZeroVersion" or "baseMinorVersion" (tried out in my diff)? Since dropping patch didn't work I am less opposed to featureVersion as that becomes intended use - just the usually naming difficulties.
Its comments need updating. There should be clarity about patch being .0. You can reference comments I was making for "baseMinorVersion" at
I think runtime-utils cleanedPackageVersion should use this instead of parse and then comments would need updated see
| * "a value that compares less than or equal to the oldest Fluid Framework client version that must be able to open and process the container". | ||
| * As a non-breaking change, we could then allow values such as "3.1" without requiring the trailing ".0". | ||
| * However, omitting ".0" might make the value look less like a version and obscure semver ordering; for example, how "3.21" is greater than "3.3". | ||
| * We may therefore want to retain the ".0" for clarity. |
There was a problem hiding this comment.
I think we can drop this or update to note that it is complicated and probably not worth it.
| export type OldestSupportedClientVersion = | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}` | ||
| | `${1 | 2 | 3}.${bigint}.${bigint}-${string}`; | ||
| | `3.${bigint}.0` |
There was a problem hiding this comment.
I had trouble commenting elsewhere since related files aren't touched. One of the comments is what I see Willie noting. I have a version of validation at
FluidFramework/packages/runtime/runtime-utils/src/compatibilityBase.ts
Lines 224 to 240 in 89a9be2
comments to address have been acknowledged
Description
Tweak how we support version 3, removing the complications that we likely shouldn't have stabilized for 1 and 2 from the new 3 support before we stabilize.
Reviewer Guidance
The review process is outlined in the pull request guidelines.