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.

Sign up for GitHub

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

Remove dictionary from ILoaderOptions #19306

Merged
merged 14 commits into from
Feb 2, 2024
Merged

Remove dictionary from ILoaderOptions #19306

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
300baee
Pull loader options out of setupClient, remove enableOfflineLoad option
ChumpChief Jan 19, 2024
bf44a5b
Drop dictionary typing of ILoaderOptions
ChumpChief Jan 19, 2024
94509a0
Add changeset
ChumpChief Jan 23, 2024
e5a715b
Merge remote-tracking branch 'upstream/main' into LessOptions
ChumpChief Jan 23, 2024
f5a78c2
Split typing for runtime side
ChumpChief Jan 23, 2024
2330e61
New changeset and revert api md change
ChumpChief Jan 23, 2024
f679b8d
Update comments
ChumpChief Jan 23, 2024
a8f184f
Prep for removing options member
ChumpChief Jan 25, 2024
6f452dd
Prep for removal of container context member on runtime side
ChumpChief Jan 25, 2024
c4ac2f4
Only read from config after connectionStateHandler is created
ChumpChief Jan 26, 2024
6f39b8d
Merge remote-tracking branch 'upstream/main' into LessOptions
ChumpChief Feb 1, 2024
45eae53
Update packages/common/container-definitions/src/loader.ts
ChumpChief Feb 1, 2024
e21dc7e
Update api md
ChumpChief Feb 1, 2024
db7fd39
Add docs
ChumpChief Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/common/container-definitions/api-report/container-definitions.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,9 +403,9 @@ export interface ILoaderHeader {

// @public (undocumented)
export type ILoaderOptions = {
[key in string | number]: any;
} & {
cache?: boolean;
client?: IClient;
enableOfflineLoad?: boolean;
provideScopeLoader?: boolean;
maxClientLeaveWaitTime?: number;
};
Expand Down
15 changes: 12 additions & 3 deletions packages/common/container-definitions/src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { IRequest, FluidObject, IEvent, IEventProvider } from "@fluidframework/core-interfaces";
import {
IClient,
IClientDetails,
IDocumentMessage,
IQuorumClients,
Expand Down Expand Up @@ -533,10 +534,8 @@ export interface IHostLoader extends ILoader {
/**
* @public
ChumpChief marked this conversation as resolved.
Show resolved Hide resolved
*/
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export type ILoaderOptions = {
Copy link
Contributor

Choose a reason for hiding this comment

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

i like removing it, but i think its used today, so we need a transition path, i don't think we can just unilaterally remove

// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key in string | number]: any;
} & {
/**
* @deprecated This option has been deprecated and will be removed in a future release
* Set caching behavior for the loader. If true, we will load a container from cache if one
Expand All @@ -548,6 +547,16 @@ export type ILoaderOptions = {
*/
cache?: boolean;

/**
* @deprecated Do not use.
*/
client?: IClient;

/**
* @deprecated Do not use.
*/
enableOfflineLoad?: boolean;

/**
* Provide the current Loader through the scope object when creating Containers. It is added
* as the `ILoader` property, and will overwrite an existing property of the same name on the
Expand Down
10 changes: 8 additions & 2 deletions packages/dds/cell/src/test/cell.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,10 @@ function createConnectedCell(
): ISharedCell {
// Create and connect a second SharedCell.
const dataStoreRuntime = new MockFluidDataStoreRuntime();
dataStoreRuntime.options = options ?? dataStoreRuntime.options;
// TODO this option shouldn't live here - this options object is global to the container
Copy link
Contributor

Choose a reason for hiding this comment

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

i think the first thing is to change the type of dataStoreRuntime be something other than ILoaderOptions, probably Record<string | number, any> to start. that would also get rid of all the casting this change introduces

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm also not positive that the province of this object actually comes all the way from the loader. the type might just be a lie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this annoying intentionally - my thought here was to discourage folks from writing new values to options (with this PR as-is, they'll get an error). This is with the goal of deterring new (possibly dangerous) usage.

I think the likely end state being that the dataStoreRuntime.options member goes away entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm also not positive that the province of this object actually comes all the way from the loader. the type might just be a lie

The loader options get shallow-copied per-Container - so shallow modifications are global to the Container, but deep modifications of true loader options (e.g. individual IClient properties) are global to the Loader. But yeah they get plumbed all the way through.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm looking at an option to split the typing (maybe this is what you originally meant with your comment) - ILoaderOptions with only known types for the Loader constructor param, but everywhere else gets JUST the record-like. Which then would point to a followup change that just severs the connection at the loader/runtime boundary, with the runtime creating an empty object as its starting options and getting nothing from the loader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in latest!

// and not specific to the individual dataStoreRuntime.
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any
dataStoreRuntime.options = (options ?? dataStoreRuntime.options) as any;

runtimeFactory.createContainerRuntime(dataStoreRuntime);
const services = {
Expand All @@ -39,7 +42,10 @@ function createConnectedCell(

function createDetachedCell(id: string, options?: ICellOptions): ISharedCell {
const dataStoreRuntime = new MockFluidDataStoreRuntime();
dataStoreRuntime.options = options ?? dataStoreRuntime.options;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-explicit-any
dataStoreRuntime.options = (options ?? dataStoreRuntime.options) as any;
const subCell = new SharedCell(id, dataStoreRuntime, CellFactory.Attributes);
return subCell;
}
Expand Down
20 changes: 15 additions & 5 deletions packages/dds/sequence/src/sequence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,9 @@ export abstract class SharedSegmentSequence<T extends ISegment>
super(id, dataStoreRuntime, attributes, "fluid_sequence_");

this.guardReentrancy =
dataStoreRuntime.options.sharedStringPreventReentrancy ?? true
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
(dataStoreRuntime.options as any).sharedStringPreventReentrancy ?? true
? ensureNoReentrancy
: createReentrancyDetector((depth) => {
if (totalReentrancyLogs > 0) {
Expand Down Expand Up @@ -277,7 +279,9 @@ export abstract class SharedSegmentSequence<T extends ISegment>
this.handle,
(op, localOpMetadata) => this.submitLocalMessage(op, localOpMetadata),
new SequenceIntervalCollectionValueType(),
dataStoreRuntime.options,
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime.options as any,
);
}

Expand Down Expand Up @@ -665,7 +669,9 @@ export abstract class SharedSegmentSequence<T extends ISegment>
.catch((error) => {
this.loadFinished(error);
});
if (this.dataStoreRuntime.options?.sequenceInitializeFromHeaderOnly !== true) {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
if ((this.dataStoreRuntime.options as any)?.sequenceInitializeFromHeaderOnly !== true) {
// if we not doing partial load, await the catch up ops,
// and the finalization of the load
await loadCatchUpOps;
Expand Down Expand Up @@ -769,15 +775,19 @@ export abstract class SharedSegmentSequence<T extends ISegment>
}
const needsTransformation = message.referenceSequenceNumber !== message.sequenceNumber - 1;
let stashMessage: Readonly<ISequencedDocumentMessage> = message;
if (this.runtime.options?.newMergeTreeSnapshotFormat !== true) {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
if ((this.runtime.options as any)?.newMergeTreeSnapshotFormat !== true) {
if (needsTransformation) {
this.on("sequenceDelta", transformOps);
}
}

this.client.applyMsg(message, local);

if (this.runtime.options?.newMergeTreeSnapshotFormat !== true) {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
if ((this.runtime.options as any)?.newMergeTreeSnapshotFormat !== true) {
if (needsTransformation) {
this.removeListener("sequenceDelta", transformOps);
// shallow clone the message as we only overwrite top level properties,
Expand Down
4 changes: 3 additions & 1 deletion packages/dds/sequence/src/sharedIntervalCollection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,9 @@ export class SharedIntervalCollection
this.handle,
(op, localOpMetadata) => this.submitLocalMessage(op, localOpMetadata),
new IntervalCollectionValueType(),
runtime.options,
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
runtime.options as any,
);
}

Expand Down
12 changes: 8 additions & 4 deletions packages/dds/sequence/src/test/fuzz/fuzzUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,14 +332,18 @@ export class SharedStringFuzzFactory extends SharedStringFactory {
services: IChannelServices,
attributes: IChannelAttributes,
): Promise<SharedString> {
runtime.options.intervalStickinessEnabled = true;
runtime.options.mergeTreeEnableObliterate = true;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
(runtime.options as any).intervalStickinessEnabled = true;
(runtime.options as any).mergeTreeEnableObliterate = true;
return super.load(runtime, id, services, attributes);
}

public create(document: IFluidDataStoreRuntime, id: string): SharedString {
document.options.intervalStickinessEnabled = true;
document.options.mergeTreeEnableObliterate = true;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
(document.options as any).intervalStickinessEnabled = true;
(document.options as any).mergeTreeEnableObliterate = true;
return super.create(document, id);
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/dds/sequence/src/test/intervalCollection.snapshot.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,11 @@ async function getSingleIntervalSummary(): Promise<{ summary: ISummaryTree; seq:
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const dataStoreRuntime = new MockFluidDataStoreRuntime();
dataStoreRuntime.local = false;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime.options = {
intervalStickinessEnabled: true,
};
} as any;
containerRuntimeFactory.createContainerRuntime(dataStoreRuntime);
const services = {
deltaConnection: dataStoreRuntime.createDeltaConnection(),
Expand Down
12 changes: 9 additions & 3 deletions packages/dds/sequence/src/test/intervalCollection.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,11 @@ describe("SharedString interval collections", () => {

// Connect the first SharedString.
dataStoreRuntime1.local = false;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime1.options = {
intervalStickinessEnabled: true,
};
} as any;
const containerRuntime1 =
containerRuntimeFactory.createContainerRuntime(dataStoreRuntime1);
const services1 = {
Expand All @@ -112,9 +114,11 @@ describe("SharedString interval collections", () => {
const dataStoreRuntime2 = new MockFluidDataStoreRuntime({ clientId: "2" });
const containerRuntime2 =
containerRuntimeFactory.createContainerRuntime(dataStoreRuntime2);
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime2.options = {
intervalStickinessEnabled: true,
};
} as any;
const services2 = {
deltaConnection: dataStoreRuntime2.createDeltaConnection(),
objectStorage: new MockStorage(),
Expand Down Expand Up @@ -1560,10 +1564,12 @@ describe("SharedString interval collections", () => {

beforeEach(() => {
dataStoreRuntime1 = new MockFluidDataStoreRuntime({ clientId: "1" });
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime1.options = {
intervalStickinessEnabled: true,
mergeTreeReferencesCanSlideToEndpoint: true,
};
} as any;
sharedString = new SharedString(
dataStoreRuntime1,
"shared-string-1",
Expand Down
4 changes: 3 additions & 1 deletion packages/dds/sequence/src/test/intervalRebasing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ function constructClients(
): [Client, Client, Client] {
return Array.from({ length: numClients }, (_, index) => {
const dataStoreRuntime = new MockFluidDataStoreRuntime();
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime.options = {
intervalStickinessEnabled: true,
mergeTreeEnableObliterate: true,
};
} as any;
const sharedString = new SharedString(
dataStoreRuntime,
String.fromCharCode(index + 65),
Expand Down
4 changes: 3 additions & 1 deletion packages/dds/sequence/src/test/overlappingSequenceIntervalsIndex.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ describe("findOverlappingIntervalsBySegoff", () => {

beforeEach(() => {
dataStoreRuntime = new MockFluidDataStoreRuntime({ clientId: "1" });
dataStoreRuntime.options = { intervalStickinessEnabled: true };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime.options = { intervalStickinessEnabled: true } as any;
testSharedString = new SharedString(
dataStoreRuntime,
"test-shared-string",
Expand Down
24 changes: 18 additions & 6 deletions packages/dds/sequence/src/test/partialLoad.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ function generateSummaryTree(
describe("SharedString Partial Load", () => {
it("Validate Full Load", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = { mergeTreeSnapshotChunkSize };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = { mergeTreeSnapshotChunkSize };
const [remoteSharedString, summaryTree] = generateSummaryTree(
containerRuntimeFactory,
options,
Expand All @@ -118,7 +120,9 @@ describe("SharedString Partial Load", () => {

it("Validate New Format Load", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = { newMergeTreeSnapshotFormat: true, mergeTreeSnapshotChunkSize };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = { newMergeTreeSnapshotFormat: true, mergeTreeSnapshotChunkSize };
const [remoteSharedString, summaryTree] = generateSummaryTree(
containerRuntimeFactory,
options,
Expand All @@ -144,7 +148,9 @@ describe("SharedString Partial Load", () => {

it("Validate Partial load", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = {
newMergeTreeSnapshotFormat: true,
sequenceInitializeFromHeaderOnly: true,
mergeTreeSnapshotChunkSize,
Expand Down Expand Up @@ -179,7 +185,9 @@ describe("SharedString Partial Load", () => {

it("Validate Partial load with local ops", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = {
sequenceInitializeFromHeaderOnly: true,
mergeTreeSnapshotChunkSize,
};
Expand Down Expand Up @@ -228,7 +236,9 @@ describe("SharedString Partial Load", () => {

it("Validate Partial load with remote ops", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = {
sequenceInitializeFromHeaderOnly: true,
mergeTreeSnapshotChunkSize,
};
Expand Down Expand Up @@ -274,7 +284,9 @@ describe("SharedString Partial Load", () => {

it("Validate Partial load with local and remote ops", async () => {
const containerRuntimeFactory = new MockContainerRuntimeFactory();
const options = {
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
const options: any = {
sequenceInitializeFromHeaderOnly: true,
mergeTreeSnapshotChunkSize,
};
Expand Down
12 changes: 9 additions & 3 deletions packages/dds/sequence/src/test/reentrancy.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ describe("SharedString op-reentrancy", () => {
const factory = SharedString.getFactory();
const dataStoreRuntime1 = new MockFluidDataStoreRuntime();
dataStoreRuntime1.local = true;
dataStoreRuntime1.options = { sharedStringPreventReentrancy };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime1.options = { sharedStringPreventReentrancy } as any;

const sharedString = factory.create(dataStoreRuntime1, "A");
sharedString.insertText(0, "abcX");
Expand Down Expand Up @@ -75,7 +77,9 @@ describe("SharedString op-reentrancy", () => {
logger: logger.toTelemetryLogger(),
});
dataStoreRuntime1.local = false;
dataStoreRuntime1.options = { sharedStringPreventReentrancy: false };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime1.options = { sharedStringPreventReentrancy: false } as any;
sharedString = factory.create(dataStoreRuntime1, "A");

const containerRuntime1 =
Expand All @@ -87,7 +91,9 @@ describe("SharedString op-reentrancy", () => {
sharedString.connect(services1);

const dataStoreRuntime2 = new MockFluidDataStoreRuntime();
dataStoreRuntime2.options = { sharedStringPreventReentrancy: false };
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime2.options = { sharedStringPreventReentrancy: false } as any;
dataStoreRuntime2.local = false;
const containerRuntime2 =
containerRuntimeFactory.createContainerRuntime(dataStoreRuntime2);
Expand Down
4 changes: 3 additions & 1 deletion packages/dds/sequence/src/test/revertibles.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,10 +1148,12 @@ describe("Sequence.Revertibles with stickiness", () => {

dataStoreRuntime1 = new MockFluidDataStoreRuntime({ clientId: "1" });
dataStoreRuntime1.local = false;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime1.options = {
intervalStickinessEnabled: true,
mergeTreeReferencesCanSlideToEndpoint: true,
};
} as any;
sharedString = stringFactory.create(dataStoreRuntime1, "shared-string-1");

const containerRuntime1 = containerRuntimeFactory.createContainerRuntime(dataStoreRuntime1);
Expand Down
4 changes: 3 additions & 1 deletion packages/framework/attributor/src/mixinAttributor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ export const mixinAttributor = (Base: typeof ContainerRuntime = ContainerRuntime

const shouldTrackAttribution = mc.config.getBoolean(enableOnNewFileKey) ?? false;
if (shouldTrackAttribution) {
(context.options.attribution ??= {}).track = true;
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
((context.options as any).attribution ??= {}).track = true;
}

const runtime = (await Base.loadRuntime({
Expand Down
4 changes: 3 additions & 1 deletion packages/framework/attributor/src/test/attribution/sharedString.attribution.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,14 @@ function createSharedString(
const initialState: FuzzTestState = {
clients: clientIds.map((clientId, index) => {
const dataStoreRuntime = new MockFluidDataStoreRuntime({ clientId });
// TODO this option shouldn't live here - this options object is global to the container
// and not specific to the individual dataStoreRuntime.
dataStoreRuntime.options = {
attribution: {
track: makeSerializer !== undefined,
policyFactory: createInsertOnlyAttributionPolicy,
},
};
} as any;
const { deltaManager } = dataStoreRuntime;
const sharedString = new SharedString(
dataStoreRuntime,
Expand Down
Loading