emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.49k stars 4.21k forks source link

[Documentation] No documented public API for `Owner` #19916

Open chriskrycho opened 2 years ago

chriskrycho commented 2 years ago

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:

https://github.com/emberjs/ember.js/blob/6b77e9b940a23698201a9d16d1fe420507f048b0/packages/%40ember/-internals/owner/index.ts#L30-L39

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

wagenet commented 2 years ago

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 commented 2 years ago

https://github.com/emberjs/rfcs/pull/821

chriskrycho commented 2 years ago

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).