-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
RFC: public types for Owner, Transition, RouteInfo #821
Changes from 1 commit
c54e797
ff376d8
c71d685
a7cc41f
4011bcf
ca1328d
1f97c1a
21ccda4
1d61643
5b9bc9c
88f13cd
42fc48a
a1b2172
e0e6df9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,358 @@ | ||
--- | ||
Stage: Accepted | ||
Start Date: 2022-05-23 | ||
Release Date: Unreleased | ||
Release Versions: | ||
ember-source: vX.Y.Z | ||
ember-data: vX.Y.Z | ||
Relevant Team(s): | ||
- Ember.js | ||
- TypeScript | ||
- Learning | ||
RFC PR: https://github.com/emberjs/rfcs/pull/821 | ||
--- | ||
|
||
# Public API for Type-Only Imports | ||
|
||
|
||
## Summary | ||
|
||
Introduce public import locations for type-only imports which have previously had no imports, and fully specify their public APIs for end users: | ||
|
||
- `Owner`, with `TypeOptions` and `FactoryManager` | ||
- `Transition` | ||
- `RouteInfo` and `RouteInfoWithAttributes` | ||
|
||
|
||
## Motivation | ||
|
||
Prior to supporting TypeScript, Ember has defined certain types as part of its API, but *without* public imports, since the types were not designed to be imported, subclassed, etc. by users. For the community-maintained type definitions, the Typed Ember team chose to match that policy so as to avoid committing the main project to public API. With the introduction of TypeScript as a first-class language (see esp. RFCs [0724: Official TypeScript Support][0724] and [0800: TypeScript Adoption Plan][0800]) those types now need public imports so that users can reference them; per the [Semantic Versioning for TypeScript Types spec][spec], they also need to define *how* they are public: can they be sub-classed, re-implemented, etc.? | ||
|
||
[0274]: https://rfcs.emberjs.com/id/0724-road-to-typescript | ||
[0800]: https://rfcs.emberjs.com/id/0800-ts-adoption-plan | ||
[spec]: https://www.semver-ts.org | ||
|
||
Additionally, the lack of a public import or contract for the `Owner` interface has been a long-standing problem for *all* users, but especially TypeScript users, and given the APIs provided for e.g. the Glimmer Component class where `owner` is the first argument, the pervasive use of `getOwner` in low-level library code, etc., it is important for TypeScript users to be able to use an `Owner` safely, and for JavaScript users to be able to get autocomplete etc. from the types we ship. | ||
|
||
|
||
## Detailed design | ||
|
||
**Note:** For the terms "user-constructible" and "user-subclassable" see the [Semantic Versioning for TypeScript Types spec][spec]. | ||
|
||
### `Owner` | ||
|
||
`Owner` is a user-constructible interface, with an intentionally-minimal subset of the : | ||
|
||
```ts | ||
export default interface Owner { | ||
rwjblue marked this conversation as resolved.
Show resolved
Hide resolved
|
||
lookup(name: string): unknown; | ||
|
||
register<T>( | ||
fullName: string, | ||
factory: FactoryManager<T>, | ||
options?: TypeOptions | ||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
): void; | ||
|
||
hasRegistration(fullName: string): boolean; | ||
|
||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
resolveRegistration( | ||
fullName: string | ||
): FactoryManager<unknown> | object | undefined; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not clear to me that we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent; will swap those! |
||
} | ||
``` | ||
|
||
It is the default import from a new module, `@ember/owner`. With it come two other new types: `TypeOptions` and `FactoryManager`. | ||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#### `TypeOptions` | ||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
`TypeOptions` is a user-constructible interface: | ||
|
||
```ts | ||
export interface TypeOptions { | ||
instantiate?: boolean | undefined; | ||
singleton?: boolean | undefined; | ||
} | ||
``` | ||
|
||
Although users will not usually need to use it directly, instead simply passing it as a POJO to `Owner#register`, it is available as a named export from `@ember/owner`: | ||
|
||
```ts | ||
import { type TypeOptions } from '@ember/owner'; | ||
``` | ||
|
||
JS users can refer to it in JSDoc comments using `import()` syntax: | ||
|
||
```js | ||
/** | ||
* @param {import('@ember/owner').TypeOptions} typeOptions | ||
*/ | ||
function useTypeOptions(typeOptions) { | ||
// ... | ||
} | ||
``` | ||
|
||
|
||
#### `FactoryManager` | ||
|
||
`FactoryManager` is an existing concept available to users via [the `Engine#factoryFor` API][ff].[^factory-name] The public API to date has included only two fields, `class` and `create`, and we maintain that in this RFC. The result is this user-constructible interface: | ||
|
||
[ff]: https://api.emberjs.com/ember/4.3/classes/EngineInstance/methods/factoryFor?anchor=factoryFor | ||
|
||
```ts | ||
export interface FactoryManager<Class> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, I actually don't think this is quite correct. The "Factory manager" is the thing that is responsible for actually making A few issues with the term "factory manager" in a public API sense:
But really, you can only actually use
All that said, I think we just need a different name than I don't think the name itself is a massive ergonomics issue, since I doubt most folks are going to actually write this name themselves (since they will interact with it as a result of calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm entirely fine with going back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rwjblue this is almost as bad as
Why in the world There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually going to call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. …okay, having read the implementation and the docs repeatedly, I now: have literally no confidence that my idea of how these types are related or how they are supposed to be used is correct. Writing this down for now and will come back to trying to actually get something reasonable after discussing with folks who know this more (@rwjblue, @wagenet, etc.) tomorrow. 😂 |
||
class: Class; | ||
create( | ||
initialValues?: { | ||
[K in keyof InstanceOf<Class>]?: InstanceOf<Class>[K]; | ||
} | ||
): InstanceOf<Class>; | ||
} | ||
``` | ||
|
||
<details><summary>The <code>InstanceOf<T></code> and <code>ClassProps</code> type</summary> | ||
|
||
The `InstanceOf<T>` type here is a utility type which is *not* exported, because it is only necessary to guarantee that `create` accepts and returns the appropriate values: the fields to set on the class instance, and the instance after construction respectively. It is provided here only for completeness. | ||
|
||
```ts | ||
type InstanceOf<T> = T extends new (...args: any) => infer R ? R : never; | ||
``` | ||
|
||
</details> | ||
|
||
`FactoryManager` is now available as a named import from `@ember/owner`: | ||
|
||
```ts | ||
import { type FactoryManager } from '@ember/owner'; | ||
``` | ||
|
||
JS users can refer to it in JSDoc comments using `import()` syntax: | ||
|
||
```js | ||
/** | ||
* @param {import('@ember/owner').FactoryManager} factoryManager | ||
*/ | ||
function useFactoryManager(factoryManager) { | ||
// ... | ||
} | ||
``` | ||
|
||
Note that the `Class` type parameter must be defined using `typeof SomeClass`, *not* `SomeClass`: | ||
|
||
```ts | ||
import { type FactoryManager } from '@ember/owner'; | ||
|
||
class Person { | ||
constructor(public name: string, public age: number) {} | ||
} | ||
|
||
class PersonManager implements FactoryManager<typeof Person> { | ||
class = Person; | ||
create({ name = "", age = 0 } = {}) { | ||
return new this.class(name, age); | ||
} | ||
} | ||
``` | ||
|
||
(This is not the actual usual internal implementation in Ember, but shows that it can be implemented safely with these types.) | ||
|
||
[^factory-name]: In Ember’s types today, this type is known as `Factory`, and the `class` is named `FactoryClass`. However, Ember’s public API docs already refer to this type as `FactoryManager`, *not* `Factory`. I have chosen `FactoryManager` to match in this RFC | ||
|
||
|
||
### `Transition` | ||
|
||
`Transition` is a non-user-constructible, non-user-subclassable class. It is identical to the *existing* public API, with two new features: | ||
|
||
- the specification of the generic type `T` representing the resolution state of the `Promise` associated with the route (and used with `RouteInfoWithAttributes`; see below) | ||
- a public import location | ||
|
||
Since it is neither constructible nor implementable, it should be supplied as type-only export. For example: | ||
|
||
```ts | ||
class _Transition<T = unknown> | ||
implements Pick<Promise<T>, 'then' | 'catch' | 'finally'> { | ||
|
||
data: Record<string, unknown>; | ||
readonly from: RouteInfoWithAttributes<T> | null; | ||
readonly promise: Promise<T>; | ||
readonly to: RouteInfo | RouteInfoWithAttributes<T>; | ||
abort(): Transition<T>; | ||
followRedirects(): Promise<T>; | ||
method(method?: string): Transition<T>; | ||
retry(): Transition<T>; | ||
trigger(ignoreFailure: boolean, name: string): void; | ||
send(ignoreFailure: boolean, name: string): void; | ||
|
||
// These names come from the Promise API, which Transition implements. | ||
then<TResult1 = T, TResult2 = never>( | ||
onfulfilled?: (value: T) => TResult1 | PromiseLike<TResult1>, | ||
onrejected?: (reason: unknown) => TResult2 | PromiseLike<TResult2>, | ||
label?: string, | ||
): Promise<TResult1 | TResult2>; | ||
catch<TResult = never>( | ||
onRejected?: (reason: unknown) => TResult | PromiseLike<TResult>, | ||
label?: string, | ||
): Promise<TResult | T>; | ||
finally(onFinally?: () => void, label?: string): Promise<T>; | ||
} | ||
|
||
export default interface Transition<T> extends _Transition<T> {} | ||
``` | ||
|
||
It is the default import from `@ember/routing/transition`: | ||
|
||
```ts | ||
import type Transition from '@ember/routing/transition'; | ||
``` | ||
|
||
JS users can refer to it in JSDoc comments using `import()` syntax: | ||
|
||
```js | ||
/** | ||
* @param {import('@ember/routing/transition').default} theTransition | ||
*/ | ||
function takesTransition(theTransition) { | ||
// ... | ||
} | ||
``` | ||
|
||
|
||
### `RouteInfo` | ||
|
||
`RouteInfo` is a non-user-constructible interface. It is identical to the *existing* public API, with the addition of a public import. | ||
|
||
```ts | ||
export default interface RouteInfo { | ||
readonly child: RouteInfo | null; | ||
readonly localName: string; | ||
readonly name: string; | ||
readonly paramNames: string[]; | ||
readonly params: Record<string, string | undefined>; | ||
readonly parent: RouteInfo | null; | ||
readonly queryParams: Record<string, string | undefined>; | ||
readonly metadata: unknown; | ||
find( | ||
callback: (item: RouteInfo, index: number, array: RouteInfo[]) => boolean, | ||
target?: unknown | ||
): RouteInfo | undefined; | ||
} | ||
``` | ||
|
||
It is the default import from `@ember/routing/route-info`: | ||
|
||
```ts | ||
import type RouteInfo from '@ember/routing/route-info'; | ||
``` | ||
|
||
JS users can refer to it in JSDoc comments using `import()` syntax: | ||
|
||
```js | ||
/** | ||
* @param {import('@ember/routing/route-info').default} routeInfo | ||
*/ | ||
function takesRouteInfo(routeInfo) { | ||
// ... | ||
} | ||
``` | ||
|
||
|
||
#### `RouteInfoWithAttributes` | ||
|
||
`RouteInfoWithAttributes` is a non-user-constructible interface, which extends `RouteInfo` by adding the `attributes` property. The attributes property represents the resolved return value from the route's model hook: | ||
|
||
```ts | ||
export interface RouteInfoWithAttributes<T = unknown> extends RouteInfo { | ||
attributes: T; | ||
} | ||
``` | ||
|
||
It is a named export from `@ember/routing/route-info`: | ||
|
||
```ts | ||
import { type RouteInfoWithAttributes } from '@ember/routing/route-info'; | ||
``` | ||
|
||
JS users can refer to it in JSDoc comments using `import()` syntax: | ||
|
||
```js | ||
/** | ||
* @param {import('@ember/routing/route-info').RouteInfoWithAttributes} routeInfo | ||
*/ | ||
function takesRouteInfoWithAttributes(routeInfoWithAttributes) { | ||
// ... | ||
} | ||
``` | ||
|
||
|
||
## How we teach this | ||
|
||
These concepts all already exist, but need updates to and in some cases wholly new pages in Ember's API docs. | ||
|
||
|
||
### `Owner` | ||
|
||
- We need to introduce API documentation in a new, dedicate module for `Owner`, `@ember/owner`. The docs on `Owner` itself should become the home for much of the documentation currently shared across the `EngineInstance` and `ApplicationInstance` classes. | ||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- We need to document the relationship between `EngineInstance` and `ApplicationInstance` as implementations of `Owner`. | ||
|
||
|
||
### `Transition`, `RouteInfo`, and `RouteInfoWithAttributes` | ||
|
||
These types need two updates to the existing API documentation: | ||
|
||
- Specify the modules they can be imported from (as noted above). | ||
- Specify that these types are *not* meant to be implemented by end users. For example: | ||
|
||
> While the `Transition` interface can be imported to name the type for use in your own code, it is not user-implementable. The only supported way to get a `Transition` is by using Ember's routing APIs, like [the `transitionTo` method](https://api.emberjs.com/ember/4.3/classes/RouterService/methods/transitionTo?anchor=transitionTo) on [the router service](https://api.emberjs.com/ember/4.3/classes/RouterService). | ||
|
||
And: | ||
|
||
> While the `RouteInfo` interface can be imported to name the type for use in your own code, it is not user-implementable. The only supported way to get a `RouteInfo` is from an of [the `Transition` class](https://api.emberjs.com/ember/4.3/classes/Transition). | ||
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
chriskrycho marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
### Blog post | ||
|
||
Besides the API docs described above and the usual discussion of new features in an Ember release blog post, we will include an extended discussion in that blog post, a blog post timed to come out around the same time as that release, or a blog post corresponding to when we add them to DefinitelyTyped, as makes the most sense. That post will situate these as part of the work done on the "road to TypeScript": | ||
|
||
- emphasizing the benefits to both JS and TS users | ||
|
||
- providing a straightforward explanation of the "non-user-constructible" constraint and refer users to the Semantic Versioning for TypeScript Types spec for more details | ||
|
||
- explaining some choices about the public `Owner` API: | ||
- that it includes the subset of the `Owner` API we want to maintain over time, *not* the full set available on the `EngineInstance` type, much of which we expect to deprecate over the course of the 4.x release cycle | ||
- that it does not include type safe registry look-ups, since we are waiting to see what TypeScript does with the Stage 3 decorators spec before deciding how to work with registries going forward[^revise] | ||
|
||
[^revise]: Depending on implementation timelines, it is possible we will revise this based on what TypeScript ships in the meantime. | ||
|
||
More general work to clarify the use of TypeScript with these APIs will be addressed as part of the forthcoming RFCs on integrating TypeScript into Ember's docs. | ||
|
||
|
||
## Drawbacks | ||
|
||
For once: none! These are already all "intimate API" *at minimum*—`Owner` is effectively public API—and the only difference between the _status quo_ and the proposed outcome is that users can know these imports won't break. Moreover, the design here is forward-compatible with any iterations on these types we can currently foresee, including expanding the type of `Owner#lookup` to support registry, changes to the `Owner` API per [RFC #0585: Improved Ember Registry APIs][0585], or future directions for the Ember routing layer. | ||
|
||
[0585]: https://rfcs.emberjs.com/id/0585-improved-ember-registry-apis | ||
|
||
|
||
## Alternatives | ||
|
||
- We could leave these in their private API locations. | ||
- We could not publish these types at all, and have users continue to use utility types to name them (`ReturnType` etc.). | ||
|
||
|
||
## Unresolved questions | ||
|
||
- Should `Owner` be exported from `@glimmer/owner` instead? Presently, that acts as only a pass-through, with the notion of an owner being delegated to implementors of the various Glimmer "manager" APIs.. | ||
|
||
- Should `Owner` have any other of its existing APIs, and in particular should it be identical to the APIs exposed via `EngineInstance`, including these additional methods? | ||
- `inject` | ||
- `ownerInjection` | ||
- `registerOptions` | ||
- `registerOptionsForType` | ||
- `registeredOption` | ||
- `registeredOptions` | ||
- `registeredOptionsForType` | ||
- `unregister` | ||
|
||
(Since `Owner` itself has never been public API, and we hope to *deprecate and remove* all of these methods in the 4.x → 5.0 era, it seems best to leave them off of `Owner` in the newly-public API, only publishing the interface we want to support long-term. | ||
|
||
|
||
- Should `Owner` be directly user-constructible, or should users be restricted to subclassing one of the existing concrete instances (`Engine` or its subclass `Application`)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Owner
is the optional first argument forEmberObject
as well.