refactor(client): make compatibility choices explicit - #28025
refactor(client): make compatibility choices explicit#28025WillieHabi wants to merge 13 commits into
Conversation
3a4b842 to
4be180a
Compare
93cf143 to
c7f882a
Compare
Jason Hartman (jason-ha)
left a comment
There was a problem hiding this comment.
This is just a partial review.
There are lots of literal versions without clarity on how they are chosen or managed.
| const tableDocumentFactory = TableDocument.getFactory(); | ||
| const runtimeFactory = new ContainerRuntimeFactoryWithDefaultDataStore({ | ||
| defaultFactory: tableDocumentFactory, | ||
| oldestSupportedClient: "3.0.0", |
There was a problem hiding this comment.
exampleOldestSupportedClient?
There was a problem hiding this comment.
So, every bump should update this?
Isn't that what pkgVersion is for?
There was a problem hiding this comment.
added its own generated pkgVersion and now use that for oldestSupportedClient so future version bumps will update this automatically.
| const runtime = await loadContainerRuntime({ | ||
| context, | ||
| existing, | ||
| oldestSupportedClient: "2.40.0", |
There was a problem hiding this comment.
Should be commented
There was a problem hiding this comment.
// createBlobPayloadPending requires runtime 2.40 or later.There was a problem hiding this comment.
Can you say a little more? Why isn't a more recent value chosen?
There was a problem hiding this comment.
Added comment:
// 2.40 is the oldest runtime that understands the document format written when
// createBlobPayloadPending is enabled; choosing a newer version would unnecessarily
// reduce the clients that this stress test can interoperate with.| ): IRuntimeFactory => { | ||
| try { | ||
| return new ctor(ctorProps); | ||
| const currentCtor = ctor as new ( |
There was a problem hiding this comment.
questionable cast.?
There was a problem hiding this comment.
Added a comment
This cast is needed because this back-compat helper receives constructors loaded from multiple package versions with different signatures, and ts can't narrow that constructor union at runtime. comment explains that we first try current object shape, then retain the existing positional fallback for older constructors.
There was a problem hiding this comment.
No version needs minVersionForCollab (for some back-compat cases)?
Because it never had proper support and only now does?
There was a problem hiding this comment.
Right sorry no historical constructuor requires minVersionForCollab. it looks like constructors before 2.0.0-internal.7.0.0 use the positional shape and arity 3. and then up to 2.116 use object props (arity 1). we verify this against all installed compat packages.
the helper now selects the constructor shape before calling it and rejects unknown arities.
| ): IRuntimeFactory => { | ||
| try { | ||
| return new ctor(ctorProps); | ||
| const currentCtor = ctor as new ( |
There was a problem hiding this comment.
No version needs minVersionForCollab (for some back-compat cases)?
Because it never had proper support and only now does?
e58641c to
8334b1d
Compare
e349f88 to
755bc3c
Compare
205d72e to
d2c51b3
Compare
36c163d to
251022a
Compare
d2c51b3 to
c542c64
Compare
fa69b50 to
e515d4a
Compare
e515d4a to
aaaaefe
Compare
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (572 lines, 95 files), I've queued these reviewers:
How this works
|
There was a problem hiding this comment.
Pull request overview
This PR refactors in-repo runtime construction to make compatibility choices explicit (primarily via oldestSupportedClient), while preserving historical default behavior for generic test and example infrastructure. It also updates Aqueduct’s legacy beta default-data-store factory props to surface the base factory’s compatibility options and wires new shared constants through tests/examples.
Changes:
- Centralize and apply historical-default compatibility constants (
defaultTestOldestSupportedClient,exampleOldestSupportedClient) across tests, tools, and examples. - Update various runtime factory call sites to pass explicit
oldestSupportedClient/minVersionForCollabwith documented floors where required (e.g.,2.40.0,2.80). - Add/adjust back-compat helper logic and tests for
ContainerRuntimeFactoryWithDefaultDataStoreconstructor shapes; update Aqueduct legacy beta API surface + changeset.
Reviewed changes
Copilot reviewed 80 out of 81 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Workspace dependency updates |
| packages/tools/fluid-runner/src/test/sampleCodeLoaders/sampleCodeLoader.ts | Explicit historical default floor |
| packages/test/test-utils/src/testContainerRuntimeFactoryWithDefaultDataStore.ts | Back-compat ctor selection logic |
| packages/test/test-utils/src/testContainerRuntimeFactory.ts | Default compatibility constant usage |
| packages/test/test-utils/src/test/testContainerRuntimeFactoryWithDefaultDataStore.spec.ts | New tests for ctor-shape helper |
| packages/test/test-utils/src/index.ts | Export defaultTestOldestSupportedClient |
| packages/test/test-utils/src/containerRuntimeFactories.ts | Centralize defaultTestOldestSupportedClient |
| packages/test/test-service-load/src/loadTestDataStore.ts | Conditional compatibility floor (2.40) |
| packages/test/test-end-to-end-tests/src/test/summarization/summarizeWithSearch.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/summarization/summarizeWithLocalChanges.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/summarization/summarizeIncrementallySubDds.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/summarization/summaries.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/stagingMode.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/pointInTime/pointInTimeTestUtils.ts | Explicit oldestSupportedClient in helpers |
| packages/test/test-end-to-end-tests/src/test/opBunching.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/offline/stashedOps.spec.ts | Explicit minVersionForCollab (2.40) |
| packages/test/test-end-to-end-tests/src/test/migration-shim/storingHandlesDetached.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/storingHandles.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/stampedV2Ops.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/sharedTreeShim.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/reconnect.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/migrationShim.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/migration-shim/dataMigration.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/localLoader.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/loadModes.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/entryPointCompat.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/deRehydrateContainerTests.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/deltaManagerProxy.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/dataStoresNested.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/data-virtualization/noGroupIdOfflineFlow.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/data-virtualization/loadNewerGroupIdSnapshot.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/data-virtualization/groupIdOffline.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/data-virtualization/groupIdInSummary.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/benchmark/DocumentMultipleDataStores.ts | Explicit oldestSupportedClient in benchmarks |
| packages/test/test-end-to-end-tests/src/test/benchmark/DocumentMatrixPlain.ts | Explicit oldestSupportedClient in benchmarks |
| packages/test/test-end-to-end-tests/src/test/benchmark/DocumentMatrix.ts | Explicit oldestSupportedClient in benchmarks |
| packages/test/test-end-to-end-tests/src/test/benchmark/DocumentMap.ts | Explicit oldestSupportedClient in benchmarks |
| packages/test/test-end-to-end-tests/src/test/attachLegacyTree.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/test-end-to-end-tests/src/test/aliasWithProps.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/utils.ts | Explicit oldestSupportedClient in loader |
| packages/test/local-server-tests/src/test/synchronousDataStoreCreation.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/stagingMode.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/sharedTreeConstraints.spec.ts | Align DDS/runtime floors (2.80) |
| packages/test/local-server-tests/src/test/readonly.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/opsOnReconnect.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/loadFrozenContainerFromPendingState.spec.ts | Conditional compatibility floor (2.40) |
| packages/test/local-server-tests/src/test/documentStagedChanges.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/documentDirty.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/data-migration/oldCode.ts | Explicit oldestSupportedClient in test code |
| packages/test/local-server-tests/src/test/data-migration/newCode.ts | Explicit oldestSupportedClient in test code |
| packages/test/local-server-tests/src/test/data-migration/basicMigration.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/connectionMode.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-tests/src/test/audience.spec.ts | Explicit oldestSupportedClient in tests |
| packages/test/local-server-stress-tests/src/test/stressDataObject.ts | Explicit oldestSupportedClient (2.40) |
| packages/test/functional-tests/src/test/containerRuntime.spec.ts | Explicit oldestSupportedClient in unit tests |
| packages/test/functional-tests/package.json | Add runtime-utils dependency |
| packages/framework/fluid-static/src/rootDataObject.ts | Use oldestSupportedClient explicitly |
| packages/framework/aqueduct/src/container-runtime-factories/containerRuntimeFactoryWithDefaultDataStore.ts | Extend base props + compat props |
| packages/framework/aqueduct/api-report/aqueduct.legacy.beta.api.md | Generated API surface update |
| packages/drivers/local-driver/src/ephemeralService.ts | Map minVersionForCollab to oldestSupportedClient |
| examples/view-integration/external-views/src/container/runtimeFactory.ts | Use exampleOldestSupportedClient |
| examples/view-integration/external-views/package.json | Depend on example-utils |
| examples/utils/migration-tools/src/compositeRuntime/loadCompositeRuntime.ts | Use exampleOldestSupportedClient |
| examples/utils/migration-tools/package.json | Depend on example-utils |
| examples/utils/example-utils/src/modelLoader/modelContainerRuntimeFactory.ts | Use exampleOldestSupportedClient |
| examples/utils/example-utils/src/index.ts | Export exampleOldestSupportedClient |
| examples/utils/example-utils/src/exampleCompatibility.ts | New shared example compat constant |
| examples/utils/example-utils/src/containerViewRuntimeFactory.ts | Use exampleOldestSupportedClient |
| examples/utils/bundle-size-tests/src/aqueduct.ts | Use exampleOldestSupportedClient |
| examples/utils/bundle-size-tests/package.json | Depend on example-utils |
| examples/data-objects/table-document/src/test/localServerTestUtils.ts | Explicit oldestSupportedClient (3.0.0) |
| examples/data-objects/smde/src/index.ts | Use exampleOldestSupportedClient |
| examples/data-objects/prosemirror/src/index.ts | Use exampleOldestSupportedClient |
| examples/data-objects/multiview/container/src/container.tsx | Use exampleOldestSupportedClient |
| examples/data-objects/codemirror/src/index.ts | Use exampleOldestSupportedClient |
| examples/apps/staging/src/container/runtimeFactory.ts | Use exampleOldestSupportedClient |
| examples/apps/claims-example/src/container/runtimeFactory.ts | Use exampleOldestSupportedClient |
| examples/apps/claims-example/package.json | Depend on example-utils |
| examples/apps/blobs/src/container/runtimeFactory.ts | Use exampleOldestSupportedClient |
| examples/apps/blobs/package.json | Depend on example-utils |
| .changeset/jolly-shoes-smile.md | Changeset for Aqueduct API update |
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.
| const tableDocumentFactory = TableDocument.getFactory(); | ||
| const runtimeFactory = new ContainerRuntimeFactoryWithDefaultDataStore({ | ||
| defaultFactory: tableDocumentFactory, | ||
| oldestSupportedClient: "3.0.0", |
There was a problem hiding this comment.
So, every bump should update this?
Isn't that what pkgVersion is for?
| const runtime = await loadContainerRuntime({ | ||
| context, | ||
| existing, | ||
| oldestSupportedClient: "2.40.0", |
There was a problem hiding this comment.
Can you say a little more? Why isn't a more recent value chosen?
| * Compatibility value for generic tests that previously relied on the runtime's implicit default. | ||
| * Keeping it centralized preserves existing test behavior and simplifies future replacement. |
There was a problem hiding this comment.
nit: this comment is written just a bit in the context of change and not in the context of anyone reading the code as it is.
There was a problem hiding this comment.
I moved the constant rewrote the comment in more steady terms:
/**
* Canonical compatibility setting for generic tests that exercise the runtime's historical
* implicit defaults. Tests for a specific feature should instead use that feature's minimum
* supported client version.
*
* @internal
*/
| oldestSupportedClient: options.createBlobPayloadPending | ||
| ? "2.40.0" | ||
| : defaultTestOldestSupportedClient, |
There was a problem hiding this comment.
To confirm what was happening here before, there was no version set so the minimum was effectively 2.40 from the option (when set) but that was never explicit in document storage?
OR was the minimum computed from the options and set in document?
Worth a note or guard for the future that when default is 2.40 or higher this can be removed?
There was a problem hiding this comment.
The runtime was not computing 2.40 from the option. With no explicit value it used 2.0.0-defaults (option-floor validation is intentionally skipped). The document stored 2.0.0-defaults as its informational minimum and separately stored createBlobPayloadPending: true in the runtime schema.
The pending-payload case now explicitly uses 2.40, while the legacy case retains 2.0.0-defaults. I added that comment beside the conditional.
| * | ||
| * @remarks | ||
| * The property name is retained while the cross-layer dual-property migration in | ||
| * {@link https://github.com/microsoft/FluidFramework/issues/27851} is completed. | ||
| */ | ||
| readonly minVersionForCollab?: OldestSupportedClientVersion; | ||
| } | ||
|
|
||
| /** | ||
| * A ContainerRuntimeFactory that initializes Containers with a single default data store, which can be requested from | ||
| * the container with an empty URL. | ||
| * | ||
| * @deprecated | ||
| * Do not reference this type directly. It will be removed in the future. | ||
| * E.g. use {@link IRuntimeFactory} instead. | ||
| */ | ||
| export class ContainerRuntimeFactoryWithDefaultDataStore |
There was a problem hiding this comment.
Joshua Smithrud (@Josmithr), what is the story for this class? It was deprecated 17 months ago. It looks like it was put into use twice in the PR with deprecation.
(WillieHabi, might be better to have your default in a different file since the suggestion is that this whole file goes away when class is not longer needed.)
There was a problem hiding this comment.
I moved it to testCompatibility.ts and left removal of the deprecated factory itself as a separate cleanup.
| ) => IRuntimeFactory) | ||
| | (new ( | ||
| props: ContainerRuntimeFactoryWithDefaultDataStoreProps & { | ||
| readonly oldestSupportedClient: OldestSupportedClientVersion; |
There was a problem hiding this comment.
oldestSupportedClient shouldn't just be a part of ContainerRuntimeFactoryWithDefaultDataStoreProps?
There may be comments lacking here and/or for ContainerRuntimeFactoryWithDefaultDataStoreProps.
There was a problem hiding this comment.
Ok yes, that is now part of ContainerRuntimeFactoryWithDefaultDataStoreProps and documented there. Callers pass defaultTestOldestSupportedClient explicitly and object constructors receive the props directly. The helper restricts the value to the historical test default.
| export type ContainerRuntimeFactoryWithDefaultDataStoreConstructor = ( | ||
| | (new ( | ||
| props: ContainerRuntimeFactoryWithDefaultDataStoreProps, | ||
| ) => IRuntimeFactory) | ||
| | (new ( | ||
| props: ContainerRuntimeFactoryWithDefaultDataStoreProps & { | ||
| readonly oldestSupportedClient: OldestSupportedClientVersion; | ||
| }, | ||
| ) => IRuntimeFactory) | ||
| | (new ( | ||
| defaultFactory: IFluidDataStoreFactory, | ||
| registryEntries: NamedFluidDataStoreRegistryEntries, | ||
| dependencyContainer?: never, | ||
| // eslint-disable-next-line import-x/no-deprecated | ||
| requestHandlers?: RuntimeRequestHandler[], | ||
| runtimeOptions?: IContainerRuntimeOptions, | ||
| provideEntryPoint?: (runtime: IContainerRuntime) => Promise<FluidObject>, | ||
| ) => IRuntimeFactory) | ||
| ) & { readonly length: number }; |
There was a problem hiding this comment.
I see a lot of duplication. Is that to avoid exporting the two sub-types?
Either just export the two-subtypes or use type filtering to extract them where needed.
Maybe these just need better specified. Associate readonly length: 1 with object versions and readonly length : 6 with the other? (I didn't know these would have a length property. Is 6 right? Or 2| 3 | 4 | 5 | 6? Where does arity 3 come from required plus first optional?)
There was a problem hiding this comment.
I simplified this so the constructor union is defined once, then derive the object + positional shapes with Extract and Exclude . Also added type guards narrow them to arity 1 or 3 so the casts are no longer needed.
The positional constructor has arity 3 because JavaScript’s Function.length stops counting at the first parameter with a default value, and requestHandlers = [] is the fourth parameter: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/length (weird)
Every installed compatibility constructor reports either 1 or 3.
| it("constructs historical object-shaped factories", () => { | ||
| let receivedProps: ContainerRuntimeFactoryWithDefaultDataStoreProps | undefined; | ||
| class HistoricalObjectFactory extends TestRuntimeFactory { |
There was a problem hiding this comment.
This seems useless.
See other comment wondering why there is no oldestSuportedClient property
There was a problem hiding this comment.
I collapsed the two object-shaped tests into one that verifies the explicit compatibility setting.
Migrate in-repo runtime factories before requiredness changes, preserve historical test behavior through shared constants, and use the current Client version consistently in examples. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Model overloaded Aqueduct constructors in compatibility test helpers and document canonical calls where the deprecated overload marks the shared symbol. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Place lint suppressions on the overloaded import specifiers so canonical calls remain clean once the deprecated compatibility overload is introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Remove suppressions that should not be required by the preferred API and preserve the historical defaults in generic fluid-runner test collateral. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Use the 2.40 compatibility floor only for option combinations that enable createBlobPayloadPending and preserve historical defaults otherwise. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Use a maintained example baseline, document intentional feature floors and test defaults, and clarify the versioned constructor compatibility cast. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
1f5caa2 to
069113d
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1ea3a180-8445-43b1-9fc0-78bbc399b8d3
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: Pending — |
Description
Moves in-repo runtime callers to explicit compatibility choices while the customer-facing APIs are still optional.
3.0.0unless a test or feature requires a documented older floor. Packages thatalready depend on
@fluid-example/example-utilsshare a common constant; other packages retainthe literal rather than adding a dependency solely for that constant.
ContainerRuntimeFactoryWithDefaultDataStorePropsadditively exposes the optional compatibility properties already supported by its base factory.This is a nonbreaking, mechanical preparation for PR #27971. It intentionally contains no snapshot-comparison changes, compatibility-validation changes, or removal of the historical default. AB#42061