Add FluidObject utility type to Replace IFluidObject

[anthony-murphy]

This change introduces the FluidObject utility type which will be a replacement for IFluidObject

fixes #8072 fixes #8073

[vladsud]

I don not like this part (.handle & IFluidLoadable loading being on same object).
To me - provider and implementer are two different concepts that should not be mixed (on consuming side) into single object / interface (i.e. IFluidLoadable should not extend IProvideFluidLoadable, and any mechanism we implement should not expose IProvideFluidLoadable & IFluidLoadable as if it were one and the same). Whoever implements IProvideFluidLoadable is free to implement both interfaces, but consumer should not assume that, allowing provider to delegate capabilities of other objects.

So when I see reference to IProvideFoo.IFoo.IFoo.IFoo above, I cringe

Here, I like IUnknonw.QueryInterface and aggregation rules - it is free to return you different objects, but there are some rules RE asking for same interface (you should get same object no matter what object from aggregations was asked for it)

[anthony-murphy]

I don't disagree with this entirely, but i don't have a solution, and think it is out of scope for the work i'm doing here. I'm really just trying to decouple the typing rats nest we get into via module augmentation and IFluidObject. In general, i'd also like to reduce our reliance on the pattern, as it should really only be used for feature discover on anonymous types. Ideally, we can get the runtime to a state where this type of pattern is not central, and we can let consumers bring their own pattern here if they need one.

specifically, I'm solving:
#2566
#3264

[anthony-murphy]

I'm considering renaming this to FluidObject, rather than Provider as i think it may make the transition easier for people.
Open to opinions

[anthony-murphy]

@vladsud and @markfields. what do you think here. i'm actually leaning towards the rename to IFluidObject, as i think it will be a less disruptive change for people.

[markfields]

I'm hoping I'm wrong, but when I try this on TS Playground, it only seems to work if IFoo extends IProvideFoo. Can you try adding tests for IFluidRunnable? That one seems to be an example where we're actually using the indirection capability of this paradigm (it doesn't extend IProvideFluidRunnable itself)

...if IFluidRunnable is even used, I didn't see any uses in our repo. Nonetheless, my understanding was that the indirection is an important feature, so would be good to account for it. Personally I haven't been able to come up with anything besides the obvious Partial<IProvideFoo> that works across the board.

[markfields]

Did some brainstorming about supporting IFoo that doesn't extend IProvideFoo. Not a polished solution, but wanted to share something that worked (TS playground):

type ProvidedBy<T> = { __providerType?: T };

type Provider<T> = 
    T extends ProvidedBy<infer P>
        ? Partial<P>
        : never;

interface IProvideFoo {
    IFoo: IFoo;
}

interface IFoo extends ProvidedBy<IProvideFoo> {
    x: number;
}

function tryit(q: unknown) {
    const maybeFoo = q as Provider<IFoo>;
    console.log(maybeFoo.IFoo?.x);
}

In cases where IFoo extends IProvideFoo, it would be nice to not need to also extend ProvidedBy and use your approach, if they can be combined in an uber Provider type somehow.

[anthony-murphy]

I'm hoping I'm wrong, but when I try this on TS Playground, it only seems to work if IFoo extends IProvideFoo. Can you try adding tests for IFluidRunnable? That one seems to be an example where we're actually using the indirection capability of this paradigm (it doesn't extend IProvideFluidRunnable itself)

...if IFluidRunnable is even used, I didn't see any uses in our repo. Nonetheless, my understanding was that the indirection is an important feature, so would be good to account for it. Personally I haven't been able to come up with anything besides the obvious Partial<IProvideFoo> that works across the board.

per the current pattern IFoo should always extend IProvideFoo. This isn't a change.

per the current pattern IFoo should always extend IProvideFoo. This isn't a change.

[markfields]

per the current pattern IFoo should always extend IProvideFoo. This isn't a change.

Really? When did this change? When I was learning about IFluidObject last summer I thought that was an important design point.

[vladsud]

per the current pattern IFoo should always extend IProvideFoo. This isn't a change.

I'm with Mark here, I think this should not be assumed or required. Provide patterns should allow aggregation, similar to COM Aggregation.

[anthony-murphy]

per the current pattern IFoo should always extend IProvideFoo. This isn't a change.

I'm with Mark here, I think this should not be assumed or required. Provide patterns should allow aggregation, similar to COM Aggregation.

It has been required due to our request pattern primarily, and is how all our interfaces have always worked, not that some aren't poorly formed, but those are not in use.

The reasons it's needed is we always supported returning an IFoo or an IProvideFoo, but since the requestor doesn't know which as the consumer, they both need the property IFoo which returns the real object. As the IFoo property is what we inspect to get at the real object. In general, we overuse the pattern in our code base where it's not necessary.

It has also always been possible to use 3 interfaces, if the consumer wishes to have an implementation only that doesn't provide itself.

Lastly, i agree with not liking the pattern. This is an incremental change to get out of the typing hell created with module augmentation, and is generally compatible with existing usage. Specifically, it is a typing only change the underlying javascript doesn't change, as the types do not exist there. Not breaking runtime javascript is an explicit goal here

Also, since these are just types, they really only exist for convivence. so users can always cast their way out of it just like they can now.

[anthony-murphy]

Did some brainstorming about supporting IFoo that doesn't extend IProvideFoo. Not a polished solution, but wanted to share something that worked (TS playground):

type ProvidedBy<T> = { __providerType?: T };

type Provider<T> = 
    T extends ProvidedBy<infer P>
        ? Partial<P>
        : never;

interface IProvideFoo {
    IFoo: IFoo;
}

interface IFoo extends ProvidedBy<IProvideFoo> {
    x: number;
}

function tryit(q: unknown) {
    const maybeFoo = q as Provider<IFoo>;
    console.log(maybeFoo.IFoo?.x);
}

In cases where IFoo extends IProvideFoo, it would be nice to not need to also extend ProvidedBy and use your approach, if they can be combined in an uber Provider type somehow.

this is a javascript breaking change

[markfields]

this is a javascript breaking change

I don't think so... it's all type and interface. The top one isn't adding an actual property to any real object. I just needed to use the generic param T somehow. It's optional so implementers can and should ignore it.

[anthony-murphy]

this is a javascript breaking change

I don't think so... it's all type and interface. The top one isn't adding an actual property to any real object. I just needed to use the generic param T somehow. It's optional so implementers can and should ignore it.

__providerType is a real property on the type, even if it is optional. This also requires all the existing interfaces to be rebuilt, which is a huge breaking change. the current change here works with existing interfaces

[anthony-murphy]

@vladsud and @markfields what do we need to close here? I'm really trying to keep this change scoped to the dependency issues created via module augmentation. The current IFluidObject is unsustainable due to this. I'm explicitly not trying to build a new discovery pattern from the one we have today.

For the discovery pattern, i'd like for that to just not be a thing that we have to own, and instead an application pattern. I think things like #8169 could help get use there

[markfields]

I think this is okay. I also wonder about telling people simply to cast to Partial<IProvideFoo>, and avoiding the complexity and limitations here.

If you do go ahead with this more complex Provider type, then I do think you should rename it to FluidObject. Biggest reason is that delegation using a separate implementation of IProvideFoo from IFoo is not supported, so it's not as flexible as "Provider" would lead me to expect. i.e. Since you need to do such a special dance to participate in this pattern, a more specific name is better.

Other feedback - I don't know that supporting FluidObject<IProvideFoo> is that valuable, do you have a case in mind from talking to Office Fluid folks? FluidObject<IFoo> reads much better (and see above about not supporting all IProvideFoo). I don't think you need to change anything in the code, just more about in terms of how it's documented/explained in comments etc.

Taking a step further... In fact, you don't need to write an IProvideFoo at all to participate here. IFoo just needs a member that returns an IFoo (and the convention to name it IFoo is nice). So if we're ditching delegation as you say, then it would be cool if we could stop publicizing anything about IProvide interfaces too, since that leads my mind towards a general-purpose delegation pattern.

Again... unless we let go of all this scaffolding and just recommend Partial<IProvideFoo> which is all good.

[markfields]

I don't understand this test scenario. Why are you giving foo.IFluidLoadable the type Provider | undefined? Shouldn't it be IFluidLoadable | undefined? And why call it unknown, instead of loadable or something?

[anthony-murphy]

this is about testing implicit conversion between an unspecified, or unknown Provider and a concrete or known Provider<IFoo>.

Think of the example of scope that the loader takes. It will take scope: Provider and i don't want to require a cast for a consumer to set scope to Provider<IFoo>. Similarly, within the runtime, when a consumer accesses scope i don't want to require a cast either, so want to support const maybeFoo: Provider<IFoo> = contenxt.scope

[anthony-murphy]

i think you are just hung up on the naming here. nothing here has to do with the typescript type unknown as it is not useful in this context

[anthony-murphy]

@vladsud and @markfields i've added a new test which verifies that the new utility type, which i've renamed from Provider to FluidObject also works if the provider is partial/optional on the implementation. This means we only need the type to specify it, and we do not need the implementation to actually have it.

While not perfect, i do think this is closer to the pattern you also prefer where provider and implementor are decoupled.

also, @barnawhi. the partial thing may make functional objects work a bit better

[markfields]

So when I try this in TS Playground, it doesn't work. And that makes sense to me - your Keys type will be doing keyof (IFoo | undefined) (line 27) which is never. Not sure why your tests pass -_-

[anthony-murphy]

weird. not sure why there is an error there, but not the test, but regardless nice find. fix it with an exclude

[markfields]

This works too:

TProp extends keyof Required<T>[TProp]

Maybe you don't have strict null checks enabled something weird like that?

[markfields]

🎂 Really cool stuff, way to go! And thanks for taking our feedback and figuring out how to support delegation too.

[markfields]

Really glad you found a good a way to accommodate proper delegation, yay! Can you update this doc comment to cover at least briefly about using Partial<IProvideFoo> if you want to do proper delegation? And I'm with Vlad - mentioning IProvideFoo.IFoo.IFoo doesn't necessarily bring clarity. That is indeed how the type magic works (it finds any prop that also exists on the prop's type), but not how this stuff is ever used.