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

Demonstrate replacing IFluidObject with unknown and using queryObject fn for feature detection #3238

Closed
wants to merge 7 commits into from

Conversation

markfields
Copy link
Member

@markfields markfields commented Aug 17, 2020

This is collateral for a post I made in Teams, about using different syntax for feature detection that is hopefully more self-descriptive and easier to learn/understand.

The gist is to change (obj as IFluidObject).IFoo to queryObject(obj).IFoo, and replace the type IFluidObject with unknown throughout. And new interfaces you want to be able to query get registered to a type called FluidDataInterfaceCatalog. See the core changes here, the rest is updating usages.

It's an incomplete change, but shows a few different places that IFluidObject appears and how it could be changed.

@tylerbutler
Copy link
Member

I feel like I'm the main one arguing with you about this. :( I'm not opposed to this syntax. It is a tiny bit more verbose than I'd like, but I can get used to it.

@tylerbutler
Copy link
Member

Nit: is there a way to get it more like object.query()? I'm guessing not. The wrapping () feels a little awkward to me.

@ghost ghost added the triage label Aug 18, 2020
@markfields
Copy link
Member Author

markfields commented Aug 18, 2020

@tylerbutler You're just first to the gate ;)

The verbosity is a feature, btw.

@markfields
Copy link
Member Author

markfields commented Aug 18, 2020

Nit: is there a way to get it more like object.query()? I'm guessing not. The wrapping () feels a little awkward to me.

Not really. If you cast it first you can do (someObject as IFluidObject).query?.IFoo, but that may defeat the purpose, and it adds a layer of indirection to the IProvide pattern, and implementations have to do some semi-awkward gymnastics to add that query: IProvideFoo property. See this branch.

Note that this is a bit of a step backwards, see https://github.com/microsoft/Prague/issues/2910

@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main August 19, 2020 02:18
@markfields
Copy link
Member Author

Ok after further discussion with @curtisman and @anthony-murphy and others on the team, I'm planning to update this proposal like so:

  • Move queryObject to a framework package as optional syntactic sugar (if it tastes sweet to you, that is)
  • Introduce type alias type FluidUnknown = object; to use instead of unknown in API signatures.
    • This keeps a special type in API signatures as a hint that you should query the thing, without carrying the huge IFluidObject type around everywhere.
    • Alias object instead of unknown since it's always going to be an object
  • Use interface FluidDataInterfaceCatalog extends Readonly<... & ...>{ } for registering new IProvide interfaces.
    • The name is much clearer for the purpose it serves.
    • Synthesize would use keyof FluidDataInterfaceCatalog instead of keyof IFluidObject
  • Leave interface IFluidObject extends Partial<FluidObjectInterfaceCatalog> { } for use as a cast target
    • Since API signatures wouldn't use IFluidObject anymore, every usage of something formerly typed as IFluidObject will need to cast. I like this since it bring the mechanism in play when you need it. And maybe some of these favor casting as Partial<IProvideFoo> directly to be more explicit.
    • We'd have to duplicate the full list of interfaces with the Interface Catalog type for a while to stage the change
    • Consider another name - QueryableFluidObject, which I think would read more explicitly?

Names of all these things are totally up for grabs, just starting somewhere.

Note that these changes are all type-level and won't change runtime behavior. Also note that this doesn't attempt to address #2566 after all, but may change the symptom if using the agnostic FluidUnknown type in API signatures removes part of the compatibility compiler error (but if the issue repros simply from importing different modules at different versions then never mind).

@curtisman
Copy link
Member

I don't think I like the existence of the queryObject at all. It is not just syntactic sugar, but has runtime implication. I agree with Tyler that this is more verbose on the user side, yet hiding inner working even more (so even more to explain when you need to tell people how it actually work).

Can you restate the problem that you are trying to solve? I am still unclear what the benefits are (at least it is not apparent to me just reading the change). Is it because the whole syntax doesn't contain the word "query" in it?

In general, I like the return value with a type that is the least generic as possible. Returning IFluidObject at least have some indication of where it could go, but just a plan unknown/object gives no type indication at all.

@markfields
Copy link
Member Author

Thanks @curtisman, good feedback. I've opened an issue where I tried to state the motivation better and describe the proposal there. I responded to Curtis's feedback there. Closing this PR out for now.

PS - I will drop queryObject, until if/when it comes up again.

@markfields markfields closed this Aug 20, 2020
@markfields markfields deleted the queryObject branch September 24, 2020 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants