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

[Documentation] No documented public API for Owner #19916

Open
chriskrycho opened this issue Jan 24, 2022 · 7 comments
Open

[Documentation] No documented public API for Owner #19916

chriskrycho opened this issue Jan 24, 2022 · 7 comments

Comments

@chriskrycho
Copy link
Contributor

As briefly discussed in #19914, there is not (and to my knowledge has never been) a definition of the public API of the Owner as expected by getOwner and setOwner. The API docs specify only Object as the type for the owner object returned by getOwner or expected as the second param to setOwner:

Additionally, while the getOwner docs include a short snippet showing the use of lookup, there is no further documentation anywhere in Ember's API docs (as far as I can tell) of what exactly an Owner is.

On #19914, @ef4 suggested that the types within Ember represent the intended public API:

export interface Owner {
lookup<T>(fullName: string, options?: LookupOptions): T | undefined;
factoryFor<T, C>(fullName: string, options?: LookupOptions): Factory<T, C> | undefined;
factoryFor(fullName: string, options?: LookupOptions): Factory<any, any> | undefined;
buildChildEngineInstance(name: string, options?: EngineInstanceOptions): EngineInstance;
register<T, C>(fullName: string, factory: Factory<T, C>, options?: object): void;
hasRegistration(name: string, options?: LookupOptions): boolean;
mountPoint?: string;
routable?: boolean;
}

If that is true, we should:

  1. Document that as a public type, so that we can
  2. Have something authoritative to point to when we open a PR to DefinitelyTyped addressing this.

Some context:

  1. Past (verbal) discussions with various folks have never been conclusive here, so I’m opening this to get us as quickly as possible to an agreed-upon answer that we can document and publish—this is causing folks some pain as they upgrade to the v4 types. Earlier versions of the types returned any here, because they predated better tools like unknown, and in the absence of a publicly-documented API, unknown (or perhaps the equally useless to end users object) are the only options on offer.

  2. Typed Ember has always maintained a policy that we do not publish types on DefinitelyTyped which do not correspond to documented public API for Ember. We are absolutely not in the business of defining the public API for Ember! Rather, we are in the business of accurately representing it in the types.

@ef4
Copy link
Contributor

ef4 commented Jan 24, 2022

IMO it hardly matters whether people originally intended it to be public. It's definitely relied on too heavily for us to ever break it without a lengthy migration plan.

There are 53 places in Ember itself that will likely blow up if anybody tries to setOwner something that's not an Owner, which you can see if you change getOwner to return unknown:

yarn run v1.22.17
$ tsc --noEmit
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts(134,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/component-managers/curly.ts(147,35): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'object | undefined'.
  Type 'unknown' is not assignable to type 'object'.
packages/@ember/-internals/glimmer/lib/component.ts(711,13): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/component.ts(712,31): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/renderer.ts(295,20): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/renderer.ts(296,15): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/renderer.ts(301,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/renderer.ts(302,19): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/renderer.ts(303,21): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/glimmer/lib/setup-registry.ts(20,17): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/glimmer/lib/views/outlet.ts(40,41): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/owner/index.ts(102,27): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'object'.
packages/@ember/-internals/routing/lib/ext/controller.ts(19,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/ext/controller.ts(20,21): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/location/auto_location.ts(183,20): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/services/router.ts(517,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(341,45): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/route.ts(449,17): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1538,17): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1544,22): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1581,31): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/route.ts(1636,9): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1637,33): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/route.ts(1642,17): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1766,31): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1820,22): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(1842,39): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/route.ts(2060,44): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(2063,9): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(2069,18): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(2082,18): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/route.ts(2094,5): error TS2322: Type 'unknown' is not assignable to type 'Owner'.
packages/@ember/-internals/routing/lib/system/router.ts(333,21): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(343,35): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(344,11): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(345,19): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(507,16): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(530,24): error TS2769: No overload matches this call.
  Overload 1 of 2, '(obj: object, keyName: never): never', gave the following error.
    Argument of type 'unknown' is not assignable to parameter of type 'object'.
  Overload 2 of 2, '(obj: object, keyName: string): unknown', gave the following error.
    Argument of type 'unknown' is not assignable to parameter of type 'object'.
packages/@ember/-internals/routing/lib/system/router.ts(627,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(628,25): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(629,25): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(630,22): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(632,7): error TS2531: Object is possibly 'null'.
packages/@ember/-internals/routing/lib/system/router.ts(633,27): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(833,30): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(1376,9): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(1379,24): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/system/router.ts(1614,30): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/router.ts(1634,30): error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Owner'.
packages/@ember/-internals/routing/lib/system/router.ts(1748,23): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/utils.ts(222,16): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/routing/lib/utils.ts(225,7): error TS2571: Object is of type 'unknown'.
packages/@ember/-internals/views/lib/system/utils.ts(121,18): error TS2571: Object is of type 'unknown'.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

So I really don't think there's any meaningful sense in which we support owners that don't conform to type.

(Unit tests of course can choose to stub only as much of the type as they actually need, but that's a given for all unit tests and doesn't really say anything about the underlying types.)

@chriskrycho
Copy link
Contributor Author

For what it's worth, I totally agree with you, but: (a) that's true of a lot of internal APIs, and (b) that therefore would normally make it “intimate” rather than public by definition. Everyone in the Typed Ember team thinks the basic Owner interface as defined should simply be public (mind, with some tweaks to get rid of unsafe casts), but that’s not our call to make. Thus the issue! 😅 If Core folks are comfortable promoting it to public API (really: acknowledging that it’s functionally already been that for ages), let’s document it and ship it.

@chriskrycho
Copy link
Contributor Author

For next steps here – @ef4 my own take is that we should go ahead and call this interface (lightly modified to remove some of the type unsafety) public. If that needs an RFC, I will write it: it's like 300 words.

@ef4
Copy link
Contributor

ef4 commented Feb 10, 2022

Yes, I think the concrete next action here could be a PR updating the docs.

@wagenet
Copy link
Member

wagenet commented Feb 10, 2022

I think we're all agreed that it should be public. It basically has to be since it is in the contractor for all EmberObject subclasses!

@wagenet
Copy link
Member

wagenet commented Jun 27, 2022

emberjs/rfcs#821

@chriskrycho
Copy link
Contributor Author

We need to implement the actual Ember side of this, but initial support has landed for the types and docs via DefinitelyTyped now that RFC 821 is merged, so: progress is happening! I expect to open a PR landing the rest of the implementation details against Ember next week (after we land #20092).

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

No branches or pull requests

3 participants