Remove dictionary from ILoaderOptions

[ChumpChief]

Effectively this change would clarify "ILoaderOptions is for known options on the Loader layer only".

Currently, ILoaderOptions permits setting any option, as it's a dictionary type. This random dictionary functionality is used by:

1. client: IClient (a long-standing secret handshake here)
2. enableOfflineLoad: boolean (a newer secret handshake)
3. A few DDS-level features, mostly sequence stuff and attribution stuff.

This change would make the first two explicit but marked deprecated (assuming their secrecy means we don't want anyone to use them). It would disallow the third category at the typing layer but avoid making functional changes immediately.

The third category seems an inappropriate use, since it's referring to a container-global options object for what are generally per-DDS settings. This seems to have been convenient for testing purposes, but seems dangerous for a customer to use (e.g. imagine two DataObjects fighting over their desired configs on the global object). Perhaps this was even done accidentally, as it's not obvious that dataStoreRuntime.options is secretly referring to a container-global options object - it really does sound like it should be options for just that dataStoreRuntime. My hope is that we can stem further usage with this change and work towards replacing the existing usage with something more appropriate.

Eventually this may allow us to remove the plumbing of the ILoaderOptions through the layers, since the specified properties aren't super interesting to the other layers.

Acknowledge that this is a breaking change (to typing, not to functionality at runtime), and I need to remember what we're doing with those now.

[anthony-murphy]

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

[anthony-murphy]

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

[ChumpChief]

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.

[ChumpChief]

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.

[ChumpChief]

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.

[ChumpChief]

Updated in latest!

[msfluid-bot]

Baseline CI build failed, cannot generate bundle analysis at this time

---

Baseline commit: 22904a9

Generated by 🚫 dangerJS against db7fd39

[ChumpChief]

@Abe27342 particularly interested to get your thought on attribution and other DDS usage, and what alternatives we might consider.

[Abe27342]

@Abe27342 particularly interested to get your thought on attribution and other DDS usage, and what alternatives we might consider.

I looked into this a while back. IMO all the DDS-level "options" should live somewhere near IFluidDataStoreRegistry in the API surface. #15397 (never merged) took a stab at that. One problem in the realm it doesn't address is that we will need clean ways to declare migrations for options which need consensus across clients. At one point Tony suggested more heavy tweaks to the data store registry pattern which might allow that kind of flow, but whatever we do will need some design work.

I know one of the stated goals of this PR is trying to discourage adding further options to ILoaderOptions which shouldn't be there, but I don't love that this also obfuscates how apps using Fluid can set these options. If we do end up with this intermediate state, I think we'll need some clear partner communication around what we're planning.

[ChumpChief]

I know one of the stated goals of this PR is trying to discourage adding further options to ILoaderOptions which shouldn't be there, but I don't love that this also obfuscates how apps using Fluid can set these options. If we do end up with this intermediate state, I think we'll need some clear partner communication around what we're planning.

My goal is not to obfuscate, but ultimately to make it illegal and stop passing loader options through to the runtime at all. If the registry-based approach works out (or another alternative) then the communication to partners would be to use that instead. Are you aware of customers using loader options to control these features already today?

[anthony-murphy]

Just pasting this here, as i think it is relevant to the discussion, but doesn't immediately impact this specific change

We currently have two different mechanisms for controlling behavior: Options and Configurations

The first and oldest are our options objects, here are some examples:
• ILoaderOptions:

FluidFramework/packages/loader/container-loader/src/loader.ts

Line 196 in 1f8c2bb

readonly options?: ILoaderOptions;

• IContainerRuntimeOptions:

FluidFramework/packages/runtime/container-runtime/src/containerRuntime.ts

Line 279 in 1f8c2bb

export interface IContainerRuntimeOptions {

There are also several dds and drivers that use a similar mechanism, specifically that mechanism is that they take some object, and that object defines behavior. Generally, these objects are hard coded by our consumers.

The second and newer is configurations. These come from a host specified configProvider and are generally dynamically accessed from somewhere. By default, we support reading them from sessionStorage for easy dev testing, and partners like FFX support configuration services like ecs and control tower. Here are some examples:
•

FluidFramework/packages/loader/driver-utils/src/blobAggregationStorage.ts

Line 175 in 611d86d

const realAllowPackaging = mc.config.getBoolean("FluidAggregateBlobs") ?? allowPacking ?? false;

•

FluidFramework/packages/drivers/odsp-driver/src/odspDocumentService.ts

Line 149 in 71cab8a

this.mc.config.getBoolean("Fluid.Driver.Odsp.binaryFormatSnapshot");

Given these two mechanisms, I thought it would be useful to start some discussion about when and where to use the different mechanisms. My perspective is the following, but hoping to discuss other’s perspectives as well:

• Options objects should only be used where it is explicitly expected and supported for consumer of our library to take advantage then long term.
• Configurations should be used for all new features. They should start off by default and be tested. In a following release be turned on by default. If no problems, they should then be removed. They are short term.
o One caveat may be using config to override options, these configs may live long term to ease testing different option values.

I think this will become quite important going forward, and changes to options objects will generally be breaking changes, whereas configs will not be, as they are not exposed on types. In general, we should stop using options objects except for features that need explicit consumer control which are few and far between.

[Abe27342]

Yeah, if plan is to check something like this in alongside an alternative way for partners to set these options then awesome, I'd love that.

I don't know exactly how our partners set all of these options, but several techniques that I've seen which don't involve passing thru loader actually have correctness issues. All the other methods I've seen basically rely on mutation of some options objects, and it's a crapshoot whether that applies before or after the option is read for whatever it affects.

E.g. one of the merge-tree flags we've since removed was mutated on construction of the dataStoreRuntime in a hasInitialized call, but when I looked into it I discovered that the option was inconsistent depending on whether you were creating or loading the DDS, and also the mutating code never ran for the summarizer client at all.

I'd guess that there are some partners which do pass in some of these as loader options.

Obviously not a great state to be in.

[Abe27342]

+1 to Tony's point on options vs config. Looking at merge-tree as a case study, I'd argue the following are more inline with "options":

• catchUpBlobName
• mergeTreeSnapshotChunkSize
• newMergeTreeSnapshotFormat (though rather than boolean this should probably be something more extensible)
• attribution

and these should be config:

• mergeTreeEnableObliterate
• mergeTreeReferencesCanSlideToEndpoint

Of the options objects, it will also make sense to document options' impact on compatibility. E.g. mergeTreeSnapshotChunkSize can probably be changed at will and different clients are free to disagree since it's just a heuristic. It so happens that the two snapshot formats for merge-tree are also compatible so that one as well, but one could imagine certain types of snapshot format changes which would have compatibility guarantees. The other options have compat implications.

[ChumpChief]

All this sounds good to me I think, though I'd also emphasize the importance of not just the mechanism (options vs. config) but more particularly the site of setting (i.e. at what layer). Anything relating to the specific contents of the runtime/DDS layer seems inappropriate to be configured from the loader layer (or the consumer of the loader layer). Conceptually it even seems odd that the consumer of a Loader would try to load some container code and say "by the way, if you happen to have any merge-trees in there I have an opinion about your snapshot chunk size".

I'd argue that these should probably be set in the runtime layer, probably by the author of the runtime factory code or DataObject code. And even if the Loader does have some unique information that should figure in ("I'm using a driver that likes 5kb chunks") then this info should instead be injected for the runtime to respond to ("As the runtime author, I'll use that preferred chunk size to tweak my merge-tree behaviors").

I'd guess that there are some partners which do pass in some of these as loader options.

Since these are all hidden/undocumented, I don't think a partner could have reasonably found these and taken a dependency unless we personally advised them to? And as you mention many are not safe to vary across loads, making it unlikely that customers with varying deployments of their loader-consuming code could have safely used them.

[Abe27342]

I'd argue that these should probably be set in the runtime layer, probably by the author of the runtime factory code or DataObject code.

Yeah, definitely agreed. Just want to emphasize that we don't have a safe way to do that today.

[anthony-murphy]

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

[anthony-murphy]

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

[anthony-murphy]

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