[FEATURE] Add new `@ember/owner` package (RFC 0821) #20271

chriskrycho · 2022-11-17T01:15:18Z

Introduce the @ember/owner package, as specified in RFC 0821: API for Type-Only Imports.

This involves a number of significant refactors, as well as a fair bit of cleanup along the way. For details; see the commits and their messages, which are fairly extensive and which I took quite a bit of time to get into a usable form for posterity. At a high level, the approach here is as follows:

First, the @ember/-internals/owner package defines both Owner, for the public API, and an InternalOwner type, which includes all of the types of Owner and more besides. To avoid circularity, @ember/-internals/owner also defines the core types for ContainerProxy and RegistryProxy, which together define Ember's actual notion of an owner in practice. (The "owner" instances Ember users interact with today are EngineInstance or its subclass ApplicationInstance; EngineInstance is created by applying ContainerProxyMixin and RegistryProxyMixin.)

The resulting type hierarchy looks like this (and, let us be clear: this is is a significant improvement because there are no module graph cycles now!):

classDiagram
  BasicRegistry <|-- Owner
  BasicRegistry <|-- RegistryProxy

  BasicContainer <|-- Owner
  BasicContainer <|-- ContainerProxy

  RegistryProxy <|-- InternalOwner
  ContainerProxy <|-- InternalOwner

  RegistryProxy <|.. RegistryProxyMixin : implements
  ContainerProxy <|.. ContainerProxyMixin : implements

  Owner <|.. EngineInstance : implements
  InternalOwner <|.. EngineInstance : implements
  RegistryProxyMixin <|-- EngineInstance : applies
  ContainerProxyMixin <|-- EngineInstance : applies

  EngineInstance <|-- ApplicationInstance

  <<Interface>> BasicRegistry
  BasicRegistry : register()

  <<Interface>> BasicContainer
  BasicContainer : lookup()
  BasicContainer : factoryFor()

  <<Interface>> Owner

  <<Interface>> RegistryProxy
  RegistryProxy : resolveRegistration()
  RegistryProxy : unregister()
  RegistryProxy : hasRegistration()
  RegistryProxy : registeredOption()
  RegistryProxy : registeredOptions()
  RegistryProxy : registerOptionsForType()
  RegistryProxy : registeredOptionsForType()

  <<Interface>> ContainerProxy
  ContainerProxy : ownerInjection()

  <<Interface>> InternalOwner

For comparison, the API we want to have in the future, and indeed the public API we have committed to for Owner—and thus a significant motivation for this refactor:

classDiagram
  class Owner {
    register()
    lookup()
    factoryFor()
  }

Critically, with these changes, the types in @ember/-internals/container and @ember/-internals/runtime depend on those in @ember/-internals/owner but not vice versa. Additionally, many other @ember/* packages can now depend solely on @ember/owner, not needing to refer to @ember/-internals at all to refer to the Owner APIs.

Second, along the same lines, @ember/-internals/owner also becomes the home of both the public definitions of types like Factory and FactoryManager and the home of internal versions which extend them, and which Ember's internals rely on. The relations here are not so tangled, so I am not supplying a diagram to go with them; the point is that in the same way there are Factory<T> and InternalFactory<T, C> extends Factory<T> and so on.

Finally, with the internals appropriately refactored, the @ember/owner package re-exports Owner and the supporting/associated types, per the RFC.

With these changes in place, we are actually (at last!) in position to do two big things:

Publish types from Ember's source: this was the last known blocker other than some build scripting. (Watch for a future PR which does just that!)
Begin eliminating all the extraneous parts of the various owner APIs, via our normal deprecation policy: since EngineInstance already implements a superset of the Owner contract (and indeed explicitly extends Owner), we can progressively eliminate the duplicate APIs.

Per [RFC 0821][rfc], this new package is the home of the `Owner` type and a number of supporting types, including the `Resolver` definition. Here, introduce the package on disk with a file which simply re-exports the existing definitions from `@ember/-internals/owner`. These definitions are *not* the final definitions: we also need to refactor the definitions within `@ember/-internals/owner` so that this package can supply *only* the public API defined in the RFC. [rfc]: https://rfcs.emberjs.com/id/0821-public-types/

This enables it to be re-exported correctly from `@ember/owner` with reference *only* to `@ember/-internals/owner`, while still providing it to the other `-internals` packages, which should never depend on the public packages like `@ember/owner`. In support of this (and many other changes), introduce a type helper for `FullName` types.

As with moving the `Resolver` definition, this aligns us with RFC 0821 and provides an easy way for `@ember/owner` to rexport the definition.

As described in an earlier commit, our public type for `Owner`, as specified by RFC 0821, is substantially narrower than the existing type we use as `Owner` internally: our *internal* notion of `Owner` is the union of `RegistryProxyMixin` and `ContainerProxyMixin`. The new `InternalOwner` type represents that union. (We intend, eventually, to carve away the additional parts of `InternalOwner` until it *is* just the specified public API for `Owner`, since that is sufficient for everything we actually do now.) In cases where the consumer does not need `InternalOwner`, switch to using this new `Owner` default export. As a knock-on, remove an `any` and actually specify a type which *also* uses `InternalOwner` internally, which involves an otherwise unrelated update to type imports and declarations.

In a number of places, fixing up the types caught issues with type safety which came down to optionality/nullability. Since the existing code *assumes* that is always valid, make that assumption visible to TS in the form of debug assertions.

Like with our distinction between `Owner` and `InternalOwner`, the notion of a `Factory` as we use it internally is somewhat richer than what we provide as a public interface. Thus, add an `InternalFactory` type which extends the public `Factory` interface, and update the internals which previously referred to `Factory` to refer to the new type instead. In a future cleanup pass, we can likely remove many of these uses of `InternalFactory` in favor of using *only* the public API for `Factory` from instead, and that will help smooth the path to removing many of these unnecessary extra details.

The use of this type allows us to provide much more type safety for both internal and external callers, by requiring that relevant strings be (dynamically) validated to have the correct shape.

While working on `Owner` refactors, take the opportunity to clarify the semantics of both types and actual behaviors in the package, and add an explicit return type to a method which was missing it.

This is the exact same kind of change previously made for `Owner` and `Factory`: providing a richer internal type for `FactoryManager` than is supported in our public API. Here, as with both of those earlier changes, there may be an opportunity to come back and switch *back* to using `FactoryManager` directly as part of a cleanup pass.

This attempts to make `.create()` type safe by constraining its argument to be `Partial<T>`.

If `DebugFactory` simply extends `Factory` with additional types, all optional, and we explicitly check and set them (as the implementation already does) then we don't need this intersection.

This has two nice effects: 1. The `KnownForTypeResult` type (janky though it is) lives where it should per RFC 0821, maintaining `@ember/-internals/owner` as the source of truth and avoiding circularity. 2. The type for `Registry` is just the class itself, with no separate interface, because there is no expectation of a difference between that concrete class type and what it expects of its own `fallback`. This commit also catches a missed import of `FactoryClass`, needed to making the `Registry` type check.

dfreeman

This looks like a solid change to me—what little feedback I have is mostly just about comment text, I think. Thank you for the thorough descriptions and commit breakdown!

packages/@ember/owner/index.ts

dfreeman · 2022-11-17T14:50:31Z

packages/@ember/-internals/container/lib/registry.ts

@@ -54,10 +43,10 @@ const VALID_FULL_NAME_REGEXP = /^[^:]+:[^:]+$/;
 @class Registry
 @since 1.11.0
 */
-export default class Registry implements IRegistry {


packages/@ember/owner/index.ts

packages/@ember/-internals/owner/index.ts

type-tests/preview/@ember/owner-tests.ts

packages/@ember/owner/index.ts

packages/@ember/-internals/owner/index.ts

These two types are fundamental to the full public notion of an "owner" in Ember: an owner *in practice* is the combination of these two types, as when defining `EngineInstance` by applying the two mixins which they correspond to. Accordingly, the definition of `InternalOwner` *must* use them... but these internal packages must also use types from the `@ember/-internals/owner` package to define their own contract; and moreover those types are *also* part of the `Owner` API contract. Accordingly, move the types to be internal to `@ember/-internals/owner` and define the mixins as simply *implementing* these interfaces. (Note: because these are mixins, the type for "implementing" the interface is defining a new interface which `extends` the interface, as usual with our mixin system.) Here, we define them in terms of extensions of`Owner`, and then define `InternalOwner` in terms of these two APIs. This needs further tweaks, as made clear by the need to use `Pick` in one of the definitions and the fact that both APIs `extends Owner` even though that isn't actually quite right; a future commit will re-align them to the correct API.

1. This does *not* deprecate the `getOwner` and `setOwner` APIs, *only* the export from `@ember/application`. 2. This is purely an in-editor signal, *not* one which will start generating runtime deprecation notices. Per Ember's usual deprecation cycle policy, we will not introduce the runtime deprecation of this API until there has been an LTS release which includes the API which replaces it (in this case, `@ember/owner`). Accordingly, this API will continue to exist throughout Ember v5, since v4.12 is the next LTS release.

This is a TSDoc-friendly/future-proofing change: most of the items which `@ember/owner` re-exports from `@ember/-internals/owner` can be documented at the definition site, because they are plain re-exports. `getOwner` is a bit different, because (as discussed in an earlier commit) the public `Owner` type it returns has much less detail than our `InternalOwner` type. We do this via a reassignment of the item, so the docs need to move to that reassignment to be visible for TS consumers and, in the future, for TSDoc extraction of docs. The existing YUIDoc infrastructure will be unaffected by this move.

An earlier commit moved the definitions for the `ContainerProxy` and `RegistryProxy` types to `@ember/-internals/owner`, noting that further refactors were needed. Introduce those further refactors: - Define `ContainerProxy` and `RegistryProxy` as core parts of `Owner`. - Introduce `BasicRegistry` type as the root on which both `Owner` and `RegistryProxy` build, so they can have a shared definition without either (incorrectly!) extending the other. - Define the `ContainerProxyMixin` and `RegistryProxyMixin` types in terms of `ContainerProxy` and `RegistryProxy` respectively. Previously, these were named in terms of mixins, but the interfaces can (and hopefully at some point *will*) be implemented by non-mixin types (or else removed entirely). - Migrate the documentation from the mixin definitions to the types. Expand on some of the documentation, especially with examples, since there was a great deal missing for making sense of these types and their relationships.

For the moment, these aim to match the type tests for the public preview types for `@ember/owner` as much as possible, but with special cases covered for the bits which are actually internal. This makes for a stronger guarantee that we will not have a breaking change for end users in the future. However, the tests for `@ember/-internals/owner` *over*-reproduce those, since they also exist for `@ember/owner` itself. A future commit will therefore remove all of the bits which are *not* specifically internal for that type test package.

Updates the preview types to include the `@ember/owner` types introduced in an earlier commit. Besides updating the other existing related exports to match the refactored types from the implementation, the refactoring involved in landing correct types internally result in a couple of key changes: 1. `getOwner()` always returns `Owner | undefined`. The previous attempt to make it return `Owner` when working with a "known object" of some sort runs into significant issues: - Ember's public API allows the direct construction of many types which are *normally* managed by the framework. It is legal, if odd and *not recommended*, to instantiate any given `Factory` with its `.create()` method: import Component from '@glimmer/component'; import Session from '../services' export default class Example extends Component { session = Session.create(); } In this example, the `session` property on `Example` will *not* have an `Owner`, so it is not safe for `session` itself to rely on that. This is annoying, but type safe, and straightforward to work around. For example, a helper designed to look up services in templates might do something like this: import Helper from '@ember/component/helper'; import { getOwner } from '@ember/owner'; import { assert } from '@ember/debug'; class GetService extends Helper { compute([serviceName]) { let owner = getOwner(this); assert('unexpected missing an owner!', !!owner); return owner.lookup(`service:${name}`); } } Then if someone did `GetService.create()` instead of using a helper in a template *or* with `invokeHelper()`, they would get a useful error message. - For the same reasons we cannot guarantee that contract from a types perspective, it is actually impossible to *implement* it in a type safe manner. 2. The service registry now *requires* that its fields be `Service` subclasses. We added this constraint as part of generalizing the DI registry system to work well with the `Owner` APIs while avoiding introducing any circularity into the module graph. This should also be future-compatible with alternative future designs. This should not be breaking in practice, because items in the service registry were always expected to be service classes. (Note that this cannot be backported to the release because the modules it represents do not exist until this whole PR lands.)

Prettier!

This means our `.create()` type does not check against the types of the target object in any way, unfortunately. However, we actually *cannot* constrain it further than this without going down a *very* deep rabbit hole (see the historic types for `.create()` on DefinitelyTyped if you're curious), because we need (for historical reasons) to support classes which implement this contract to be able to provide a *narrower* interface than "exactly the public fields on the class" while still falling back to the "exactly the public fields on the class" for the general case. Unfortunately, Ember internally relies on the ability for items which implement `Factory` to be able to customize the arguments to the `.create()` call in just that way.

This is a bit annoying: the fixer actually makes the declaration *worse* by some interpretations. But 🤷

In `CoreObject.create()`, we currently have an unsafe cast when constructing an instance with props, because the code assumes one of two things. Either: the `factory` is always set, *or* it is safe to call `setFactoryFor` with `undefined`. A number of acceptance tests are (incorrectly? Unclear!) relying on the ability to run through this path with `factory` being `undefined`. It's *possible* that actually means that the type for `setFactoryFor` should allow `undefined`, but we typed it the other way for good reason! Accordingly, this *casts* `factory`, and the commented-out `assert()` is here in the hope that we can enable it after addressing tests in the future.

The `generateControllerFactory()` function *should* require that the class associated with the `FactoryManager` for a given controller name be a `Controller` subclass, but the tests today do *not* keep this contract. We should fix that, but *separately* from this types-focused change, which aims to change *only* the types and module graph.

The private `Router._setOutlets()` method *should* require that there be an actual `-application-instance:main` available, since otherwise it literally won't work at runtime. However, at least one test runs without it set correctly: Router Service - non application test: RouterService#transitionTo with basic route We should fix that, but *separately* from this types-focused change, which aims to change *only* the types and module graph.

wagenet

Partial review.

packages/@ember/-internals/glimmer/lib/resolver.ts

wagenet · 2022-11-17T21:18:48Z

packages/@ember/-internals/glimmer/lib/views/outlet.ts

+    environment: BootEnvironment;
+    application: InternalOwner;
+    template: TemplateFactory;
+  }): OutletView {


Goodbye any!

wagenet · 2022-11-17T21:22:00Z

packages/@ember/application/index.ts

@@ -30,6 +30,16 @@ import RouterService from '@ember/routing/router-service';
 import type { EngineInstanceOptions } from '@ember/engine/instance';
 import type { SimpleDocument, SimpleElement } from '@simple-dom/interface';

+/**
+ * @deprecated Use `import { getOwner } from '@ember/owner';` instead.


Should we runtime deprecate this function as well?

Yes, but I wanted to land that separately from this—both because it's safer and keeps the types/behavior separately, and also (perhaps more importantly) because we usually try not to turn on the deprecation in the same release as we put out the replacement for it, because otherwise people get hammered with deprecations they haven't had a chance to do fix, including from addons.

chriskrycho · 2022-11-18T21:04:13Z

All right, going to go ahead and merge this, and if/as folks catch issues we'll fix bugs!

wagenet · 2022-11-17T22:53:32Z

packages/@ember/engine/instance.ts

+  // This is effectively an "abstract" method: it defines the contract a
+  // subclass (e.g. `ApplicationInstance`) must follow to implement this
+  // behavior, but an `EngineInstance` has no behavior of its own here.
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars


Does ESLint complain about them even when they're underscored? Should we change this?

It does, and we probably should for things like this at least.

wagenet · 2022-11-17T22:56:01Z

packages/@ember/routing/router.ts

      if (instance) {
-        instance.didCreateRootView(this._toplevelView);
+        // SAFETY: LOL. This is calling a deprecated API with a type that we


Pretty much!

wagenet · 2022-11-17T22:56:57Z

type-tests/preview/@ember/owner-tests.ts

@@ -35,19 +41,19 @@ aFactory.create({
  hasProps: false,
 });

-// These should be rejected by way of EPC
-// @ts-expect-error
+// NOTE: it would be nice if these could be rejected by way of EPC, but alas: it


“Excess Property Checking”: the feature where if you pass an object literal as an argument with extra properties than the parameter type specifies, TS will error since that is basically guaranteed to e incorrect.

Integrate the service registry into the `DIRegistry` introduced as part of rationalizing the `Owner` types in PR #20271 (94276b5). This allows `Owner.lookup('service:foo')` to resolve a `FooService` if one is set up in the `@ember/service` module's `Registry` interface. The preview types already used this mechanic, so this just means that when we ship the stable (i.e. built from source) version of `@ember/service`, it will *continue* working. Meta: Shipping this implementation for the lookup was blocked on being able to publish type modules with `declare module`, which was implemented in PR #20316 (9adcd15). We will likely need to rework other parts of the type hierarchy to support publishing from source.

Integrate the service registry into the `DIRegistry` introduced as part of rationalizing the `Owner` types in PR #20271 (94276b5). This allows `Owner.lookup('service:foo')` to resolve a `FooService` if one is set up in the `@ember/service` module's `Registry` interface. The preview types already used this mechanic (as of 5658b13), so this just means that when we ship the stable (i.e. built from source) version of `@ember/service`, it will *continue* working. Meta: Shipping this implementation for the lookup was blocked on being able to publish type modules with `declare module`, which was implemented in PR #20316 (9adcd15). We will likely need to rework other parts of the type hierarchy to support publishing from source.

Integrate the service registry into the `DIRegistry` introduced as part of rationalizing the `Owner` types in PR #20271 (94276b5). This allows `Owner.lookup('service:foo')` to resolve a `FooService` if one is set up in the `@ember/service` module's `Registry` interface. The preview types already used this mechanic (as of 5658b13), so this just means that when we ship the stable (i.e. built from source) version of `@ember/service`, it will *continue* working. Meta: Shipping this implementation for the lookup was blocked on being able to publish type modules with `declare module`, which was implemented in PR #20316 (9adcd15). We will likely need to rework other parts of the type hierarchy to support publishing from source. (cherry picked from commit 5070508)

chriskrycho added 7 commits November 16, 2022 17:18

Move TypeOptions to @ember/-internals/owner as RegisterOptions

bbacfdb

As with moving the `Resolver` definition, this aligns us with RFC 0821 and provides an easy way for `@ember/owner` to rexport the definition.

squash w/whatever adds KnownForTypeResult

337118e

Fix some incorrect @module comments

3b01283

Debug-fail on type-unsafety with asserts

50e6aac

In a number of places, fixing up the types caught issues with type safety which came down to optionality/nullability. Since the existing code *assumes* that is always valid, make that assumption visible to TS in the form of debug assertions.

chriskrycho mentioned this pull request Nov 17, 2022

[WIP/Draft] Add new @ember/owner package (RFC 0821) #20259

Closed

6 tasks

chriskrycho requested a review from wagenet November 17, 2022 01:15

chriskrycho force-pushed the rfc821-pt3 branch 2 times, most recently from 9285a94 to 0c8f2fa Compare November 17, 2022 14:11

chriskrycho added 8 commits November 17, 2022 07:32

Combine some imports for clarity

6ddeeec

Propagate FullName type throughout the owner system

414aa6c

The use of this type allows us to provide much more type safety for both internal and external callers, by requiring that relevant strings be (dynamically) validated to have the correct shape.

@ember/engine/instance: fix a lint, add some comments

c29244a

While working on `Owner` refactors, take the opportunity to clarify the semantics of both types and actual behaviors in the package, and add an explicit return type to a method which was missing it.

Improve type safety for .create()

f855303

This attempts to make `.create()` type safe by constraining its argument to be `Partial<T>`.

Unify Factory and DebugFactory types in container

95ba2a5

If `DebugFactory` simply extends `Factory` with additional types, all optional, and we explicitly check and set them (as the implementation already does) then we don't need this intersection.

chriskrycho force-pushed the rfc821-pt3 branch 2 times, most recently from 97dcb03 to 8172773 Compare November 17, 2022 14:35

dfreeman reviewed Nov 17, 2022

View reviewed changes

chriskrycho commented Nov 17, 2022

View reviewed changes

packages/@ember/owner/index.ts Outdated Show resolved Hide resolved

chriskrycho commented Nov 17, 2022

View reviewed changes

packages/@ember/-internals/owner/index.ts Outdated Show resolved Hide resolved

chriskrycho added 6 commits November 17, 2022 09:22

Update @ember/application type tests for new @ember/owner types

d01a89c

chriskrycho added 11 commits November 17, 2022 09:22

Fix white space in comments in @ember/controller

63fb7cf

Prettier!

EngineInstance explicitly implements Owner

3af52bc

Fix lint errors in Owner packages

7e49f29

Fix a type error in CoreObject on TS@next

b2ce803

Fix another linting issue in @ember/engine

2340d2d

This is a bit annoying: the fixer actually makes the declaration *worse* by some interpretations. But 🤷

Fix explanatory comments in @ember/-internals/owner

b08757b

chriskrycho force-pushed the rfc821-pt3 branch from 19e9f8c to b08757b Compare November 17, 2022 16:23

wagenet reviewed Nov 17, 2022

View reviewed changes

chriskrycho merged commit 737af5c into master Nov 18, 2022

chriskrycho deleted the rfc821-pt3 branch November 18, 2022 21:04

wagenet reviewed Nov 18, 2022

View reviewed changes

SergeAstapov mentioned this pull request Nov 24, 2022

@ember/application: Expand FrameworkObject DefinitelyTyped/DefinitelyTyped#63368

Closed

17 tasks

chriskrycho mentioned this pull request Nov 29, 2022

Advance RFC #0821 to Stage Ready for Release emberjs/rfcs#874

Merged

10 tasks

chriskrycho mentioned this pull request Dec 9, 2022

[BUGFIX beta] Types: resolve services with Owner.lookup #20319

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FEATURE] Add new `@ember/owner` package (RFC 0821) #20271

[FEATURE] Add new `@ember/owner` package (RFC 0821) #20271

chriskrycho commented Nov 17, 2022 •

edited

Loading

dfreeman left a comment

dfreeman Nov 17, 2022

wagenet left a comment

wagenet Nov 17, 2022

wagenet Nov 17, 2022

chriskrycho Nov 17, 2022

chriskrycho commented Nov 18, 2022

wagenet Nov 17, 2022

chriskrycho Nov 19, 2022

wagenet Nov 17, 2022

chriskrycho Nov 19, 2022

wagenet Nov 17, 2022

chriskrycho Nov 19, 2022

[FEATURE] Add new @ember/owner package (RFC 0821) #20271

[FEATURE] Add new @ember/owner package (RFC 0821) #20271

Conversation

chriskrycho commented Nov 17, 2022 • edited Loading

dfreeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wagenet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriskrycho commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[FEATURE] Add new `@ember/owner` package (RFC 0821) #20271

[FEATURE] Add new `@ember/owner` package (RFC 0821) #20271

chriskrycho commented Nov 17, 2022 •

edited

Loading