Lock down ways to instantiate a DDS

[markfields]

Feature

Is your feature request related to a problem? Please describe.
Looking at the code for several DDS's, I'm seeing a pattern where these 3 are exposed publicly:

public static create(runtime: IComponentRuntime, ...) { ... }

public static getFactory() { ... }

public constructor(...) { ... }

There are clear patterns of use, but for a new dev walking up to these classes, it's confusing what you're supposed to do.

Describe the solution you'd like
It seems that the only one that should be exposed publicly is public static create.

The constructor should only be callable by the Factory, but I'm not sure how to actually implement this in TypeScript.

As for getFactory, a few ideas:

1. Have the callers the factory directly (replace SharedMap.getFactory() with new MapFactory())
2. Replace getFactory with registerFactory which takes in a function (2a) or type (2b) that can take the factory and do the right thing with it - without exposing a trivial way to get the factory returned.
   2a. Define a type/interface like this: type FactoryRegistrar = (factory: ISharedObjectFactory) => void; then registerFactory is:

    public static registerFactory(registrar: FactoryRegistrar) {
        registrar(new ConsensusRegisterCollectionFactory());
    }

And callers become:

        const dataTypes = new Map<string, ISharedObjectFactory>();
        const registrar = (f: ISharedObjectFactory) => dataTypes.set(f.type, f);
        ConsensusRegisterCollection.registerFactory(registrar);

2b. Add register(factory: ISharedObjectFactory); to ISharedObjectRegistry and provide a class that implements it like this:

    register(factory: ISharedObjectFactory) {
        this.map.set(factory.type, factory);
    }

Then callers look like this:

        const dataTypes = new SharedObjectRegistry();
        ConsensusRegisterCollection.registerFactory(dataTypes);

[vladsud]

FluidDataStoreRuntime.createChannel() seems like the other (right?) way to create DDS, though it lacks type safety.
As we are attempting to move to type-safe usage of create-then-attach flow, it feels like createChannel() should not be on public API surface, and instead creation should always go through DDS itself (or its factory).

[vladsud]

Here is what I think the world looks like (using SharedMap as an example):

1. SharedMap.getFactory() is only for adding DDS factory to data store registry.
2. constructor should not be used directly (likely should be always private, but there is no "friend" keyword in TS to make it available only to factory)
3. SharedMap.create() - the only thing that does over IFluidDataStoreRuntime.createChannel() is casting to SharedMap.
4. IFluidDataStoreRuntime.createChannel() has to exists as that's the core of creation code.

We do want to push people toward # 3 pattern - this story still is in progress for data stores, but it was always the case for DDS, so it's good.

So I think the only interesting tidbit is # 1 - usage of factories. I do not see how your proposal will play out, given typical code we write looks like that:

export class Clicker extends DataObject implements IFluidHTMLView {
    private static readonly factory = new DataObjectFactory<Clicker, undefined, undefined, IEvent>(
        Clicker.ComponentName,
        Clicker,
        [
            SharedCounter.getFactory(),
            SharedMap.getFactory(),
        ],
        {});
}

I do not think the following is better - it takes more effort to track things as they are now in two different places:

const dataTypes = new SharedObjectRegistry();
SharedCounter.registerFactory(dataTypes);
SharedMap.registerFactory(dataTypes),

....

export class Clicker extends DataObject implements IFluidHTMLView {
    private static readonly factory = new DataObjectFactory<Clicker, undefined, undefined, IEvent>(
        Clicker.ComponentName,
        Clicker,
        dataTypes,
        {});
}

I'd suggest to do nothing here (i.e. punt this issue) and wait for #3469 (Unification of channels) to eventually change this area.

[ghost]

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

[markfields]

@vladsud / @ChumpChief - Worth keeping open and investing in? I don't have much depth of knowledge in this space, it was just something I noticed while exploring the code early on.

[ghost]

This issue has been automatically marked as stale because it has had no activity for 180 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!