[WIP/Draft] Add new @ember/owner package (RFC 0821)#20259
Closed
chriskrycho wants to merge 23 commits intomasterfrom
Closed
[WIP/Draft] Add new @ember/owner package (RFC 0821)#20259chriskrycho wants to merge 23 commits intomasterfrom
@ember/owner package (RFC 0821)#20259chriskrycho wants to merge 23 commits intomasterfrom
Conversation
chriskrycho
force-pushed
the
rfc821-pt3
branch
30 times, most recently
from
1e60017 to
a875c66
Compare
chriskrycho
changed the title
@ember/owner package (RFC 0821)
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.
# This is the 1st commit message: Distinguish `Factory` and `InternalFactory` 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 commit message #2 will be skipped: # InternalFactory needs Factory :sigh:
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.
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.
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.)
chriskrycho
force-pushed
the
rfc821-pt3
branch
from
a875c66 to
a0c4e52
Compare
Contributor
Author
|
Closing this in favor of a new PR with a useful history. 😅 |
chriskrycho
changed the title
@ember/owner package (RFC 0821)@ember/owner package (RFC 0821)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement the runtime and stable types side of RFC 0821: API for Type-Only Imports for
@ember/owner.SUPERCEDED: #20271
Tasks
@ember/ownerpackage with the public API specified.