Skip to content
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

[BUGFIX release] Fix types for getOwner and GlimmerComponent #20257

Merged
merged 3 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this, but it's probably worth it to avoid having to assert every time we getOwner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I’m going to merge with this as is, but I’m also going to do a follow-on which actually makes it safer, based on something I came up with chatting @dfreeman this morning.

export function getOwner(object: unknown): Owner | undefined;
/**
* `setOwner` forces a new owner on a given object instance. This is primarily
Expand Down