Add TChannel to IChannelFactory, and related cleanups

[CraigMacomber]

Description

Add TChannel type parameter to IChannelFactory, and a few related cleanups.

Changes split out from #19717

Breaking Changes

Add TChannel type parameter (which defaults to IFluidLoadable) to IChannelFactory. When left at its default this preserves the old behavior, however packages exporting IChannelFactory will now reference IFluidLoadable if not providing a different parameter and thus will implicitly depend on @fluidframework/core-interfaces

SharedCounter.create now returns ISharedCounter instead of SharedCounter

Reviewer Guidance

The review process is outlined on this wiki page.

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +22 Bytes

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	517.45 KB	517.46 KB	⯅ +11 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	250.94 KB	250.94 KB	■ No change
loader.js	127.97 KB	127.97 KB	■ No change
map.js	42.89 KB	42.89 KB	■ No change
matrix.js	145.03 KB	145.03 KB	■ No change
odspDriver.js	97.21 KB	97.21 KB	■ No change
odspPrefetchSnapshot.js	41.91 KB	41.91 KB	■ No change
sharedString.js	163.23 KB	163.24 KB	⯅ +11 Bytes
sharedTree.js	332.14 KB	332.14 KB	■ No change
Total Size	1.82 MB	1.82 MB	⯅ +22 Bytes

---

Baseline commit: b6bf9d0

Generated by 🚫 dangerJS against 0394b02

[Abe27342]

why constrain to and default as IFluidLoadable rather than IChannel? Seems like it would give simpler types.

[CraigMacomber]

I was unable to make the type inference around isDataObject work correctly when I did that (its what I tried first, and I tried it again when making this PR). A lot of code is generic over shared object and data objects, and parameterized by a IFluidLoadable: making shared objects require I channel seems like it should work, but you end up needing an extends clause or another intersection in the LoadableObject union, and that screws up narrowing. Its likley possible to make work, but I tried and failed twice, so I went with this which works fine and is a smaller delta.

[Abe27342]

Hm. I can see how you'd run into trouble trying to extend typesafety to fluid-static based on how those APIs are set up, but I'm not seeing failures locally by only changing IChannelFactory. It seems like that puts us closer to where we'd eventually like things than what's in the PR.

Concretely, seems like this PR + this change builds fine: commit

and it's only once we start trying to extend that typesafety to non-encapuslated model APIs where things start to get trickier: commit, motivating idea being we'd like L62 to specify the return type param for its IChannelFactory, but this forces us to muck around with a bunch of extends clauses. (the specific mucking I've done there ends up not working because it doesn't do the reasonable thing for LoadableOjectClass<any> which is used elsewhere in that file, but that's not very important).

[CraigMacomber]

My experiments with this were before I swapped the order of the LoadableObjectClass union. Let me see if it works better now that that has been swapped.

[anthony-murphy]

i got it to work with a single cast in RootDataObject.create. its necessary due to limitations of type narrowing on generics:
main...anthony-murphy:prototype-ichannel-in-factory

The end typing is also a bit wonky, as i went with interfaces in the factories, which results in union of class and interface due to the dependency of contianer schema on class. this could be resolve by just using the class as the generic parameter to the factory. it may also be possible to parametrize the factory itself, so it works with either classes or interfaces to keep the ability to more away from classes more easily supported.

[CraigMacomber]

I tried integrating that change into my AzureFactory branch, and it broke tons of tests using container schema.

The issue is that if:

export type LoadableObjectClass<T extends IFluidLoadable = IFluidLoadable> =
	| (T extends IChannel ? SharedObjectClass<T> : never)
	| DataObjectClass<T>;

Then LoadableObjectClass<IFluidLoadable> simplifies to DataObjectClass which means that SharedObjectClass no longer can be used in ContainerSchema.

In short, that version of LoadableObjectClass fails to be covariant with respect to T, which breaks a lot of things including shared objects in container schema.

An interesting feature of the approach I took (& IChannel on the return ) is that it now becomes optional for interfaces like ITree to extend IChannel, so we could actually hide (in the container schema based API) the fact that trees expose things like "getAttachSummary", "summarize", "getGCData" if we want to.

[anthony-murphy]

why make that change at all, can't it remain:
export type LoadableObjectClass<T extends IFluidLoadable>

[CraigMacomber]

My above comment was in response to Abram's proposal, but @anthony-murphy 's approach:

export type LoadableObjectClass<T extends IFluidLoadable> = T extends IChannel
	? SharedObjectClass<T>
	: DataObjectClass<T>;

Is one I tried previously, and actually has the same issue where LoadableObjectClass simplifies to DataObjectClass which means that SharedObjectClass no longer can be used in ContainerSchema when combined with the changes in my AzureFactory branch.

This change also has the issue that it becomes impossible to properly support a data-object that returns a type which happens to implement IChannel: the old code, my changes and Abram's suggestions all allowed that case. THis change results in my adjusted

export function isDataObjectClass<T extends IFluidLoadable>(
	obj: LoadableObjectClass<T>,
): obj is InternalDataObjectClass<T>;

failing to compile, and needing to be changed to:

export function isDataObjectClass<T extends IFluidLoadable>(
	obj: LoadableObjectClass<T> | DataObjectClass<T>,
): obj is InternalDataObjectClass<T>;

If we are willing to keep the use of any in ContainerSchema (which results in untyped container schema returning DDSs and shared objects as any instead of as IFluidLoadable as is remove in my other branch as a follow-up to this work) then I suspect we could make this work, but that seems worse than what I have proposed which avoids the cast, avoids the use of any, and only looks a little funny if you read the implementation of IChannelFactory, but nothing about its unsafe from a type perspective.

The current implementation of InitialObjects does produce any with Anthony's version, but the one on my branch does work around such issues. Thus it's possible to work around the lack of covariance, but evert user of the container schema type would need to do so, and forgetting to do so will produce any and the workaround is messy and a bit fragile.

I think factory style APIs should maintain covariant and thus be able to avoid any. My approach accomplishes this while the other suggestions here do not.

I have moved some of changes (removal of any) from my other branch into this one which benefit from the covariance and added a private doc comment about this design choice.

[Abe27342]

I still think my first proposal builds and solves things for the encapsulated model in isolation. But I didn't realize your other changes actually do introduce typesafety to the azure APIs, at which point I agree the end state is better--seems reasonable to me.

[tyler-cai-microsoft]

I know this may be out of scope, I'm liking this PR already. I wonder if we could add types to ISharedDirectory<T> and ISharedMap<T>

That would be awesome :).

[CraigMacomber]

There are complexities with doing that since that T might want to be changed over time, which brings in compatibility issues. SharedTree is building quite a lot of stuff to properly support something like that, but I think maybe applying the actual type parameter belongs in a data object not the DDS for some nuanced reasons around enabling flexibility for handling compatibility errors. Also such typing really should be parried with persisting type information in the document to validate you don't get compatibility errors on accident: SharedTree also does this.

So in short, yes we should aim for that, but its persisted data so its complicated and needs a good compatibility and schema migration story. I'll make sure tree delivers that in a way that works cleanly with container schema, then maybe we can cover the other DDSes.

[Josmithr]

Devtools changes look fine to me.