Skip to content

Commit

Permalink
Merge pull request #20257 from emberjs/fix-types-FrameworkObject
Browse files Browse the repository at this point in the history
[BUGFIX release] Fix types for `getOwner` and GlimmerComponent
  • Loading branch information
chriskrycho authored Nov 8, 2022
2 parents 0580eed + dff8831 commit 698674b
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 2 deletions.
28 changes: 28 additions & 0 deletions type-tests/preview/@ember/owner-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import Owner, {
Resolver,
KnownForTypeResult,
} 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';

// Just a class we can construct in the Factory and FactoryManager tests
declare class ConstructThis {
Expand Down Expand Up @@ -143,6 +147,30 @@ typedStrings.map((aString) => owner.lookup(aString));
const aConstName = 'type:name';
expectTypeOf(owner.lookup(aConstName)).toBeUnknown();

// Check handling with Glimmer components carrying a Signature: they should
// properly resolve to `Owner`, *not* `Owner | undefined`.
interface Sig<T> {
Args: {
name: string;
age: number;
extra: T;
};
Element: HTMLParagraphElement;
Blocks: {
default: [greeting: string];
extra: [T];
};
}

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

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

// ----- Minimal further coverage for POJOs ----- //
// `Factory` and `FactoryManager` don't have to deal in actual classes. :sigh:
const Creatable = {
Expand Down
18 changes: 16 additions & 2 deletions types/preview/@ember/application/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,29 @@ declare module '@ember/application' {
// 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.
type FrameworkObject = EmberObject | GlimmerComponent;
//
// 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.
*/
export function getOwner(object: FrameworkObject): Owner;
// 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;
/**
* `setOwner` forces a new owner on a given object instance. This is primarily
Expand Down

0 comments on commit 698674b

Please sign in to comment.