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.

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
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
16 changes: 16 additions & 0 deletions .changeset/slick-kings-wish.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
"@fluidframework/container-definitions": major
"@fluidframework/container-loader": major
"@fluidframework/container-runtime": major
"@fluidframework/container-runtime-definitions": major
"@fluidframework/datastore": major
"@fluidframework/datastore-definitions": major
"@fluidframework/runtime-definitions": major
"@fluidframework/sequence": major
"@fluid-private/test-end-to-end-tests": major
"@fluidframework/test-runtime-utils": major
---

ILoaderOptions no longer accepts arbitrary key/value pairs

ILoaderOptions has been narrowed to the specific set of supported loader options, and may no longer be used to pass arbitrary key/value pairs through to the runtime.
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export interface IContainerContext {
// (undocumented)
readonly loader: ILoader;
// (undocumented)
readonly options: ILoaderOptions;
readonly options: Record<string | number, any>;
// (undocumented)
pendingLocalState?: unknown;
// (undocumented)
Expand Down 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
16 changes: 12 additions & 4 deletions packages/common/container-definitions/src/loader.ts
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 @@ -303,7 +304,6 @@ export type ConnectionState =
* The Host's view of a Container and its connection to storage
* @alpha
*/
// eslint-disable-next-line import/no-deprecated
export interface IContainer extends IEventProvider<IContainerEvents> {
/**
* The Delta Manager supporting the op stream for this Container
Expand Down Expand Up @@ -533,10 +533,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 +546,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
6 changes: 4 additions & 2 deletions packages/common/container-definitions/src/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { IAudience } from "./audience";
import { IDeltaManager } from "./deltas";
import { ICriticalContainerError } from "./error";
import { ILoader, ILoaderOptions } from "./loader";
import { ILoader } from "./loader";
import { IFluidCodeDetails } from "./fluidPackage";

/**
Expand Down Expand Up @@ -123,7 +123,9 @@ export interface IBatchMessage {
* @alpha
*/
export interface IContainerContext {
readonly options: ILoaderOptions;
// Used to be ILoaderOptions, this is staging for eventual removal.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
readonly options: Record<string | number, any>;
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be a separate PR that decouples context from loader options. that would be non-breaking

Copy link
Contributor

Choose a reason for hiding this comment

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

that could include container runtime, and data store runtime too, bascially decouple the lower layer first

ChumpChief marked this conversation as resolved.
Show resolved Hide resolved
readonly clientId: string | undefined;
readonly clientDetails: IClientDetails;
readonly storage: IDocumentStorageService;
Expand Down
6 changes: 3 additions & 3 deletions packages/dds/sequence/src/sequence.ts
Original file line number Diff line number Diff line change
Expand Up @@ -665,7 +665,7 @@ export abstract class SharedSegmentSequence<T extends ISegment>
.catch((error) => {
this.loadFinished(error);
});
if (this.dataStoreRuntime.options?.sequenceInitializeFromHeaderOnly !== true) {
if (this.dataStoreRuntime.options.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 +769,15 @@ 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) {
if (this.runtime.options.newMergeTreeSnapshotFormat !== true) {
if (needsTransformation) {
this.on("sequenceDelta", transformOps);
}
}

this.client.applyMsg(message, local);

if (this.runtime.options?.newMergeTreeSnapshotFormat !== true) {
if (this.runtime.options.newMergeTreeSnapshotFormat !== true) {
if (needsTransformation) {
this.removeListener("sequenceDelta", transformOps);
// shallow clone the message as we only overwrite top level properties,
Expand Down
26 changes: 14 additions & 12 deletions packages/loader/container-loader/src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,7 @@ export class Container
private _loadedFromVersion: IVersion | undefined;
private attachStarted = false;
private _dirtyContainer = false;
private readonly offlineLoadEnabled: boolean;
private readonly savedOps: ISequencedDocumentMessage[] = [];
private baseSnapshot?: ISnapshotTree;
private baseSnapshotBlobs?: ISerializableBlobContents;
Expand Down Expand Up @@ -675,12 +676,8 @@ export class Container
return this._clientId;
}

private get offlineLoadEnabled(): boolean {
const enabled =
this.mc.config.getBoolean("Fluid.Container.enableOfflineLoad") ??
this.options?.enableOfflineLoad === true;
// summarizer will not have any pending state we want to save
return enabled && this.deltaManager.clientDetails.capabilities.interactive;
private get isInteractiveClient(): boolean {
return this.deltaManager.clientDetails.capabilities.interactive;
}

/**
Expand Down Expand Up @@ -824,7 +821,7 @@ export class Container

this.client = Container.setupClient(
this._containerId,
this.options,
options.client,
this.clientDetailsOverride,
);

Expand Down Expand Up @@ -941,6 +938,10 @@ export class Container
pendingLocalState?.clientId,
);

this.offlineLoadEnabled =
this.mc.config.getBoolean("Fluid.Container.enableOfflineLoad") ??
options.enableOfflineLoad === true;

this.on(savedContainerEvent, () => {
this.connectionStateHandler.containerSaved();
});
Expand Down Expand Up @@ -1630,7 +1631,8 @@ export class Container
this.baseSnapshotBlobs = pendingLocalState.snapshotBlobs;
} else {
assert(snapshot !== undefined, 0x237 /* "Snapshot should exist" */);
if (this.offlineLoadEnabled) {
// non-interactive clients will not have any pending state we want to save
if (this.offlineLoadEnabled && this.isInteractiveClient) {
this.baseSnapshot = snapshot;
// Save contents of snapshot now, otherwise closeAndGetPendingLocalState() must be async
this.baseSnapshotBlobs = await getBlobContentsFromTree(
Expand Down Expand Up @@ -2032,13 +2034,12 @@ export class Container

private static setupClient(
containerId: string,
options?: ILoaderOptions,
loaderOptionsClient?: IClient,
clientDetailsOverride?: IClientDetails,
): IClient {
const loaderOptionsClient = structuredClone(options?.client);
const client: IClient =
loaderOptionsClient !== undefined
? (loaderOptionsClient as IClient)
? structuredClone(loaderOptionsClient)
anthony-murphy marked this conversation as resolved.
Show resolved Hide resolved
: {
details: {
capabilities: { interactive: true },
Expand Down Expand Up @@ -2343,7 +2344,8 @@ export class Container
}

private processRemoteMessage(message: ISequencedDocumentMessage) {
if (this.offlineLoadEnabled) {
// non-interactive clients will not have any pending state we want to save
if (this.offlineLoadEnabled && this.isInteractiveClient) {
this.savedOps.push(message);
}
const local = this.clientId === message.clientId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import { IEventProvider } from '@fluidframework/core-interfaces';
import { IFluidDataStoreContextDetached } from '@fluidframework/runtime-definitions';
import { IFluidHandle } from '@fluidframework/core-interfaces';
import { IFluidHandleContext } from '@fluidframework/core-interfaces';
import { ILoaderOptions } from '@fluidframework/container-definitions';
import { IProvideFluidDataStoreRegistry } from '@fluidframework/runtime-definitions';
import { IRequest } from '@fluidframework/core-interfaces';
import { IResponse } from '@fluidframework/core-interfaces';
Expand All @@ -41,7 +40,7 @@ export interface IContainerRuntime extends IProvideFluidDataStoreRegistry, ICont
getAliasedDataStoreEntryPoint(alias: string): Promise<IFluidHandle<FluidObject> | undefined>;
readonly isDirty: boolean;
// (undocumented)
readonly options: ILoaderOptions;
readonly options: Record<string | number, any>;
// (undocumented)
readonly scope: FluidObject;
// (undocumented)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License.
*/

import { AttachState, IDeltaManager, ILoaderOptions } from "@fluidframework/container-definitions";
import { AttachState, IDeltaManager } from "@fluidframework/container-definitions";
import {
IEventProvider,
IRequest,
Expand Down Expand Up @@ -57,7 +57,8 @@ export type IContainerRuntimeBaseWithCombinedEvents = IContainerRuntimeBase &
export interface IContainerRuntime
extends IProvideFluidDataStoreRegistry,
IContainerRuntimeBaseWithCombinedEvents {
readonly options: ILoaderOptions;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
readonly options: Record<string | number, any>;
readonly clientId: string | undefined;
readonly clientDetails: IClientDetails;
readonly connected: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import { IGarbageCollectionData } from '@fluidframework/runtime-definitions';
import { IGetPendingLocalStateProps } from '@fluidframework/container-definitions';
import type { IIdCompressor } from '@fluidframework/id-compressor';
import type { IIdCompressorCore } from '@fluidframework/id-compressor';
import { ILoaderOptions } from '@fluidframework/container-definitions';
import { IProvideFluidHandleContext } from '@fluidframework/core-interfaces';
import { IQuorumClients } from '@fluidframework/protocol-definitions';
import { IRequest } from '@fluidframework/core-interfaces';
Expand Down Expand Up @@ -180,7 +179,7 @@ export class ContainerRuntime extends TypedEventEmitter<IContainerRuntimeEvents
// (undocumented)
notifyOpReplay(message: ISequencedDocumentMessage): Promise<void>;
// (undocumented)
readonly options: ILoaderOptions;
readonly options: Record<string | number, any>;
// (undocumented)
orderSequentially<T>(callback: () => T): T;
// (undocumented)
Expand Down
7 changes: 4 additions & 3 deletions packages/runtime/container-runtime/src/containerRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
IRuntime,
ICriticalContainerError,
AttachState,
ILoaderOptions,
ILoader,
LoaderHeader,
IGetPendingLocalStateProps,
Expand Down Expand Up @@ -934,7 +933,7 @@ export class ContainerRuntime
return runtime;
}

public readonly options: ILoaderOptions;
public readonly options: Record<string | number, any>;
private imminentClosure: boolean = false;

private readonly _getClientId: () => string | undefined;
Expand Down Expand Up @@ -1245,7 +1244,9 @@ export class ContainerRuntime
this.submitSummaryFn = submitSummaryFn;
this.submitSignalFn = submitSignalFn;

this.options = options;
// TODO: After IContainerContext.options is removed, we'll just create a new blank object {} here.
// Values are generally expected to be set from the runtime side.
this.options = options ?? {};
this.clientDetails = clientDetails;
this.isSummarizerClient = this.clientDetails.type === summarizerClientType;
this.loadedFromVersionId = context.getLoadedFromVersion()?.id;
Expand Down
10 changes: 3 additions & 7 deletions packages/runtime/container-runtime/src/dataStoreContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,7 @@ import {
IFluidHandle,
ITelemetryProperties,
} from "@fluidframework/core-interfaces";
import {
IAudience,
IDeltaManager,
AttachState,
ILoaderOptions,
} from "@fluidframework/container-definitions";
import { IAudience, IDeltaManager, AttachState } from "@fluidframework/container-definitions";
import { TypedEventEmitter } from "@fluid-internal/client-utils";
import { assert, Deferred, LazyPromise } from "@fluidframework/core-utils";
import { IDocumentStorageService } from "@fluidframework/driver-definitions";
Expand Down Expand Up @@ -145,7 +140,8 @@ export abstract class FluidDataStoreContext
return this.pkg;
}

public get options(): ILoaderOptions {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public get options(): Record<string | number, any> {
return this._containerRuntime.options;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { IFluidLoadable } from '@fluidframework/core-interfaces';
import { IGarbageCollectionData } from '@fluidframework/runtime-definitions';
import { IIdCompressor } from '@fluidframework/id-compressor';
import { IInboundSignalMessage } from '@fluidframework/runtime-definitions';
import { ILoaderOptions } from '@fluidframework/container-definitions';
import { IQuorumClients } from '@fluidframework/protocol-definitions';
import { ISequencedDocumentMessage } from '@fluidframework/protocol-definitions';
import { ISummaryTreeWithStats } from '@fluidframework/runtime-definitions';
Expand Down Expand Up @@ -117,7 +116,7 @@ export interface IFluidDataStoreRuntime extends IEventProvider<IFluidDataStoreRu
// (undocumented)
readonly objectsRoutingContext: IFluidHandleContext;
// (undocumented)
readonly options: ILoaderOptions;
readonly options: Record<string | number, any>;
// (undocumented)
readonly rootRoutingContext: IFluidHandleContext;
submitSignal(type: string, content: any, targetClientId?: string): void;
Expand Down
10 changes: 3 additions & 7 deletions packages/runtime/datastore-definitions/src/dataStoreRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,7 @@ import {
IFluidHandle,
FluidObject,
} from "@fluidframework/core-interfaces";
import {
IAudience,
IDeltaManager,
AttachState,
ILoaderOptions,
} from "@fluidframework/container-definitions";
import { IAudience, IDeltaManager, AttachState } from "@fluidframework/container-definitions";
import {
IDocumentMessage,
IQuorumClients,
Expand Down Expand Up @@ -53,7 +48,8 @@ export interface IFluidDataStoreRuntime
readonly channelsRoutingContext: IFluidHandleContext;
readonly objectsRoutingContext: IFluidHandleContext;

readonly options: ILoaderOptions;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
readonly options: Record<string | number, any>;

readonly deltaManager: IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;

Expand Down
3 changes: 1 addition & 2 deletions packages/runtime/datastore/api-report/datastore.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import { IFluidHandleContext } from '@fluidframework/core-interfaces';
import { IGarbageCollectionData } from '@fluidframework/runtime-definitions';
import { IIdCompressor } from '@fluidframework/id-compressor';
import { IInboundSignalMessage } from '@fluidframework/runtime-definitions';
import { ILoaderOptions } from '@fluidframework/container-definitions';
import { IQuorumClients } from '@fluidframework/protocol-definitions';
import { IRequest } from '@fluidframework/core-interfaces';
import { IResponse } from '@fluidframework/core-interfaces';
Expand Down Expand Up @@ -96,7 +95,7 @@ export class FluidDataStoreRuntime extends TypedEventEmitter<IFluidDataStoreRunt
// (undocumented)
get objectsRoutingContext(): this;
// (undocumented)
readonly options: ILoaderOptions;
readonly options: Record<string | number, any>;
// (undocumented)
process(message: ISequencedDocumentMessage, local: boolean, localOpMetadata: unknown): void;
// (undocumented)
Expand Down
10 changes: 3 additions & 7 deletions packages/runtime/datastore/src/dataStoreRuntime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,7 @@ import {
IResponse,
} from "@fluidframework/core-interfaces";
import { assert, Deferred, LazyPromise, unreachableCase } from "@fluidframework/core-utils";
import {
IAudience,
IDeltaManager,
AttachState,
ILoaderOptions,
} from "@fluidframework/container-definitions";
import { IAudience, IDeltaManager, AttachState } from "@fluidframework/container-definitions";
import { buildSnapshotTree } from "@fluidframework/driver-utils";
import {
IClientDetails,
Expand Down Expand Up @@ -176,7 +171,8 @@ export class FluidDataStoreRuntime
private readonly pendingHandlesToMakeVisible: Set<IFluidHandle> = new Set();

public readonly id: string;
public readonly options: ILoaderOptions;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
public readonly options: Record<string | number, any>;
public readonly deltaManager: IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;
private readonly quorum: IQuorumClients;
private readonly audience: IAudience;
Expand Down
Loading
Loading