Skip to content

Commit

Permalink
Update preview types to match the implementation
Browse files Browse the repository at this point in the history
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.)
  • Loading branch information
chriskrycho committed Nov 17, 2022
1 parent ecd3db7 commit 5658b13
Show file tree
Hide file tree
Showing 9 changed files with 402 additions and 143 deletions.
2 changes: 1 addition & 1 deletion type-tests/preview/@ember/application-test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ declare class MyService extends Service {
withStuff: true;
}
declare let myService: MyService;
expectTypeOf(getOwner(myService)).toEqualTypeOf<Owner>();
expectTypeOf(getOwner(myService)).toEqualTypeOf<Owner | undefined>();

// @ts-expect-error
getOwner();
Expand Down
80 changes: 50 additions & 30 deletions type-tests/preview/@ember/owner-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,18 @@ import Owner, {
RegisterOptions,
Resolver,
KnownForTypeResult,
getOwner,
setOwner,
} from '@ember/owner';
import Component from '@glimmer/component';
import { expectTypeOf } from 'expect-type';
// TODO: once we move the runtime export to `@ember/owner`, update this import
// as well. (That's why it's tested in this module!)
import { getOwner } from '@ember/application';
import {
getOwner as getOwnerApplication,
setOwner as setOwnerApplication,
} from '@ember/application';

expectTypeOf(getOwnerApplication).toEqualTypeOf(getOwner);
expectTypeOf(setOwnerApplication).toEqualTypeOf(setOwner);

// Just a class we can construct in the Factory and FactoryManager tests
declare class ConstructThis {
Expand All @@ -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
// cannot, because the public contract for `create` allows implementors to
// define their `create` config object basically however they like. :-/
aFactory.create({ unrelatedNonsense: 'yep yep yep' });
// @ts-expect-error
aFactory.create({ hasProps: true, unrelatedNonsense: 'yep yep yep' });

// But this should be legal.
const goodPojo = { hasProps: true, unrelatedNonsense: 'also true' };
aFactory.create(goodPojo);

// while this should be rejected for *type error* reasons, not EPC
// This should also be rejected, though for *type error* reasons, not EPC; alas,
// it cannot, for the same reason.
const badPojo = { hasProps: 'huzzah', unrelatedNonsense: 'also true' };
// @ts-expect-error
aFactory.create(badPojo);

// ----- FactoryManager ----- //
Expand All @@ -56,26 +62,29 @@ expectTypeOf(aFactoryManager.class).toEqualTypeOf<Factory<ConstructThis>>();
expectTypeOf(aFactoryManager.create({})).toEqualTypeOf<ConstructThis>();
expectTypeOf(aFactoryManager.create({ hasProps: true })).toEqualTypeOf<ConstructThis>();
expectTypeOf(aFactoryManager.create({ hasProps: false })).toEqualTypeOf<ConstructThis>();
// @ts-expect-error

// Likewise with these.
aFactoryManager.create({ otherStuff: 'nope' });
// @ts-expect-error
aFactoryManager.create({ hasProps: true, otherStuff: 'nope' });
expectTypeOf(aFactoryManager.create(goodPojo)).toEqualTypeOf<ConstructThis>();
// @ts-expect-error
aFactoryManager.create(badPojo);

// ----- Resolver ----- //
declare let resolver: Resolver;
expectTypeOf<Resolver['normalize']>().toEqualTypeOf<((fullName: FullName) => string) | undefined>();
expectTypeOf<Resolver['normalize']>().toEqualTypeOf<
((fullName: FullName) => FullName) | undefined
>();
expectTypeOf<Resolver['lookupDescription']>().toEqualTypeOf<
((fullName: FullName) => string) | undefined
>();
expectTypeOf(resolver.resolve('some-name')).toEqualTypeOf<object | Factory<object> | undefined>();
expectTypeOf(resolver.resolve('random:some-name')).toEqualTypeOf<
object | Factory<object> | undefined
>();
const knownForFoo = resolver.knownForType?.('foo');
expectTypeOf(knownForFoo).toEqualTypeOf<KnownForTypeResult<'foo'> | undefined>();
expectTypeOf(knownForFoo?.['foo:bar']).toEqualTypeOf<boolean | undefined>();
// @ts-expect-error
knownForFoo?.['blah'];
// @ts-expect-error -- there is no `blah` on `knownForFoo`, *only* `foo`.
knownForFoo?.blah;

// This one is last so it can reuse the bits from above!
// ----- Owner ----- //
Expand All @@ -88,12 +97,16 @@ expectTypeOf(owner.lookup('type:name')).toEqualTypeOf<unknown>();
owner.lookup('non-namespace-string');
expectTypeOf(owner.lookup('namespace@type:name')).toEqualTypeOf<unknown>();

declare module '@ember/service' {
interface Registry {
'my-type-test-service': ConstructThis;
// Arbitrary registration patterns work, as here.
declare module '@ember/owner' {
export interface DIRegistry {
etc: {
'my-type-test': ConstructThis;
};
}
}
expectTypeOf(owner.lookup('service:my-type-test-service')).toEqualTypeOf<ConstructThis>();

expectTypeOf(owner.lookup('etc:my-type-test')).toEqualTypeOf<ConstructThis>();

expectTypeOf(owner.register('type:name', aFactory)).toEqualTypeOf<void>();
expectTypeOf(owner.register('type:name', aFactory, {})).toEqualTypeOf<void>();
Expand All @@ -117,17 +130,17 @@ expectTypeOf(
owner.register('non-namespace-string', aFactory);
expectTypeOf(owner.register('namespace@type:name', aFactory)).toEqualTypeOf<void>();

expectTypeOf(owner.factoryFor('type:name')).toEqualTypeOf<FactoryManager<unknown> | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.class).toEqualTypeOf<Factory<unknown> | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.create()).toEqualTypeOf<unknown>();
expectTypeOf(owner.factoryFor('type:name')?.create({})).toEqualTypeOf<unknown>();
expectTypeOf(
owner.factoryFor('type:name')?.create({ anythingGoes: true })
).toEqualTypeOf<unknown>();
expectTypeOf(owner.factoryFor('type:name')).toEqualTypeOf<FactoryManager<object> | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.class).toEqualTypeOf<Factory<object> | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.create()).toEqualTypeOf<object | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.create({})).toEqualTypeOf<object | undefined>();
expectTypeOf(owner.factoryFor('type:name')?.create({ anythingGoes: true })).toEqualTypeOf<
object | undefined
>();
// @ts-expect-error
owner.factoryFor('non-namespace-string');
expectTypeOf(owner.factoryFor('namespace@type:name')).toEqualTypeOf<
FactoryManager<unknown> | undefined
FactoryManager<object> | undefined
>();

// Tests deal with the fact that string literals are a special case! `let`
Expand Down Expand Up @@ -171,12 +184,12 @@ interface Sig<T> {

class ExampleComponent<T> extends Component<Sig<T>> {
checkThis() {
expectTypeOf(getOwner(this)).toEqualTypeOf<Owner>();
expectTypeOf(getOwner(this)).toEqualTypeOf<Owner | undefined>();
}
}

declare let example: ExampleComponent<string>;
expectTypeOf(getOwner(example)).toEqualTypeOf<Owner>();
expectTypeOf(getOwner(example)).toEqualTypeOf<Owner | undefined>();

// ----- Minimal further coverage for POJOs ----- //
// `Factory` and `FactoryManager` don't have to deal in actual classes. :sigh:
Expand All @@ -185,7 +198,14 @@ const Creatable = {
};

const pojoFactory: Factory<typeof Creatable> = {
create(initialValues?) {
// If you want *real* safety here, alas: you cannot have it. The public
// contract for `create` allows implementors to define their `create` config
// object basically however they like. As a result, this is the safest version
// possible: Making it be `Partial<Thing>` is *compatible* with `object`, and
// requires full checking *inside* the function body. It does not, alas, give
// any safety *outside* the class. A future rationalization of this would be
// very welcome.
create(initialValues?: Partial<typeof Creatable>) {
const instance = Creatable;
if (initialValues) {
if (initialValues.hasProps) {
Expand Down
2 changes: 1 addition & 1 deletion type-tests/preview/ember/ember-module-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ expectTypeOf(
Ember.getEngineParent(new Ember.EngineInstance())
).toEqualTypeOf<Ember.EngineInstance>();
// getOwner
expectTypeOf(Ember.getOwner(new Ember.Component())).toEqualTypeOf<Owner>();
expectTypeOf(Ember.getOwner(new Ember.Component())).toEqualTypeOf<Owner | undefined>();
// getProperties
expectTypeOf(Ember.getProperties({ z: 23 }, 'z').z).toEqualTypeOf<number>();
expectTypeOf(Ember.getProperties({ z: 23 }, 'z', 'z').z).toEqualTypeOf<number>();
Expand Down
32 changes: 7 additions & 25 deletions types/preview/@ember/application/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,41 +116,23 @@ declare module '@ember/application' {
buildInstance(options?: object): ApplicationInstance;
}

// Known framework objects, so that `getOwner` can always, accurately, return
// `Owner` when working with one of these classes, which the framework *does*
// guarantee will always have an `Owner`. NOTE: this must be kept up to date
// whenever we add new base classes to the framework. For example, if we
// introduce a standalone `Service` or `Route` base class which *does not*
// extend from `EmberObject`, it will need to be added here.
//
// NOTE: we use `any` here because we need to make sure *not* to fix the
// actual GlimmerComponent type; using `unknown` or `{}` or `never` (the
// obvious alternatives here) results in a version which is too narrow, such
// that any subclass which applies a signature does not get resolved by the
// definition of `getOwner()` below.
type KnownFrameworkObject = EmberObject | GlimmerComponent<any>;

/**
* Framework objects in an Ember application (components, services, routes, etc.)
* are created via a factory and dependency injection system. Each of these
* objects is the responsibility of an "owner", which handled its
* instantiation and manages its lifetime.
*
* @deprecated Use `import { getOwner } from '@ember/owner';` instead.
*/
// SAFETY: this first overload is, strictly speaking, *unsafe*. It is possible
// to do `let x = EmberObject.create(); getOwner(x);` and the result will *not*
// be `Owner` but instead `undefined`. However, that's quite unusual at this
// point, and more to the point we cannot actually distinguish a `Service`
// subclass from `EmberObject` at this point: `Service` subclasses `EmberObject`
// and adds nothing to it. Accordingly, if we want to catch `Service`s with this
// (and `getOwner(this)` for some service will definitely be defined!), it has
// to be this way. :sigh:
export function getOwner(object: KnownFrameworkObject): Owner;
export function getOwner(object: unknown): Owner | undefined;
export function getOwner(object: object): Owner | undefined;

/**
* `setOwner` forces a new owner on a given object instance. This is primarily
* useful in some testing cases.
*
* @deprecated Use `import { setOwner } from '@ember/owner';` instead.
*/
export function setOwner(object: unknown, owner: Owner): void;
export function setOwner(object: object, owner: Owner): void;

/**
* Detects when a specific package of Ember (e.g. 'Ember.Application')
Expand Down
10 changes: 2 additions & 8 deletions types/preview/@ember/engine/-private/container-proxy-mixin.d.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
declare module '@ember/engine/-private/container-proxy-mixin' {
import Owner from '@ember/owner';
import { ContainerProxy } from '@ember/owner';
import Mixin from '@ember/object/mixin';

/**
* Given a fullName return a factory manager.
*/
interface ContainerProxyMixin extends Owner {
/**
* Returns an object that can be used to provide an owner to a
* manually created instance.
*/
ownerInjection(): {};
}
interface ContainerProxyMixin extends ContainerProxy {}
const ContainerProxyMixin: Mixin;
export default ContainerProxyMixin;
}
46 changes: 2 additions & 44 deletions types/preview/@ember/engine/-private/registry-proxy-mixin.d.ts
Original file line number Diff line number Diff line change
@@ -1,54 +1,12 @@
declare module '@ember/engine/-private/registry-proxy-mixin' {
import Owner from '@ember/owner';
import { RegistryProxy } from '@ember/owner';
import Mixin from '@ember/object/mixin';

/**
* RegistryProxyMixin is used to provide public access to specific
* registry functionality.
*/
interface RegistryProxyMixin extends Owner {
/**
* Given a fullName return the corresponding factory.
*/
resolveRegistration(fullName: string): unknown;
/**
* Unregister a factory.
*/
unregister(fullName: string): unknown;
/**
* Check if a factory is registered.
*/
hasRegistration(fullName: string): boolean;
/**
* Register an option for a particular factory.
*/
registerOption(fullName: string, optionName: string, options: {}): unknown;
/**
* Return a specific registered option for a particular factory.
*/
registeredOption(fullName: string, optionName: string): {};
/**
* Register options for a particular factory.
*/
registerOptions(fullName: string, options: {}): unknown;
/**
* Return registered options for a particular factory.
*/
registeredOptions(fullName: string): {};
/**
* Allow registering options for all factories of a type.
*/
registerOptionsForType(type: string, options: {}): unknown;
/**
* Return the registered options for all factories of a type.
*/
registeredOptionsForType(type: string): {};
/**
* Define a dependency injection onto a specific factory or all factories
* of a type.
*/
inject(factoryNameOrType: string, property: string, injectionName: string): unknown;
}
interface RegistryProxyMixin extends RegistryProxy {}
const RegistryProxyMixin: Mixin;
export default RegistryProxyMixin;
}
3 changes: 2 additions & 1 deletion types/preview/@ember/engine/instance.d.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
declare module '@ember/engine/instance' {
import { FullName } from '@ember/owner';
import ContainerProxyMixin from '@ember/engine/-private/container-proxy-mixin';
import RegistryProxyMixin from '@ember/engine/-private/registry-proxy-mixin';
import EmberObject from '@ember/object';
Expand All @@ -11,7 +12,7 @@ declare module '@ember/engine/instance' {
/**
* Unregister a factory.
*/
unregister(fullName: string): unknown;
unregister(fullName: FullName): unknown;

/**
* Initialize the `EngineInstance` and return a promise that resolves
Expand Down
Loading

0 comments on commit 5658b13

Please sign in to comment.