Tweak support for 3.x in OldestSupportedClientVersion - #28022
Tweak support for 3.x in OldestSupportedClientVersion#28022Craig Macomber (Microsoft) (CraigMacomber) wants to merge 5 commits into
Conversation
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (80 lines, 16 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
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: unresolved The PR's CI build failed — fix the build and the comment will update once the next run succeeds. |
| 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.
| // eslint-disable-next-line import-x/no-internal-modules -- featureVersion is alpha and has no alpha subpath export | ||
| import { featureVersion } from "@fluidframework/driver-definitions/internal"; |
There was a problem hiding this comment.
Add /alpha then?
There was a problem hiding this comment.
That API is only supposed to be used from fluid-framework according to our docs. Adding an entry point forcing it to exist in a place we say not to use it from seems harmful rather than helpful as it stabilizes the export location (or would when its promoted). I don't like making implementation details of which package something is defined in a user facing thing.
I guess since this is end-to-end tests it could depend on fluid-framework though.
There was a problem hiding this comment.
f-f dep seems better. appropriate for tests
| 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.
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.