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

Conversation

chriskrycho
Copy link
Contributor

The type for getOwner() is intended to return Owner in cases where we can safely assume that the object instance does in fact have an owner. However, as pointed out by @bendemboski today in Discord, this does not work correctly in the scenario where we have a component which actually has a signature defined:

class Example extends Component<{ Args: { anyWillDo: true } }> {
  method() {
    getOwner(this); // should be `Owner`, not `| undefined`
  }
}

Presently, we return Owner | undefined instead.

This PR fixes this by using GlimmerComponent<any> instead of plain GlimmerComponent, since the latter resolves to the default GlimmerComponent<unknown>, which ends up being too narrow to resolve correctly with getOwner when any actual signature is applied.

As a bonus, rename FrameworkObject in this context to the somewhat more verbose KnownFrameworkObject, which avoids a conflict with the actual FrameworkObject type used internally.


Additionally: The type for getOwner() is technically unsafe, but this unsafety is one which end users will rarely hit in practice and, courtesy of Ember's current types, is unavoidable if getOwner(someService) is to correctly return Owner instead of Owner | undefined.

The Typed Ember folks have discussed this point at some length, but I realized on looking at this to make the changes in the first two commits for this PR that we didn't have it committed to the code base anywhere, so this PR also introduces a long comment explaining it. When we switch from preview to stable types, this comment should come along!

This catches the scenario where we have a component which actually has
a signature defined:

    class Example extends Component<{ Args: { anyWillDo: true } }> {
      method() {
        getOwner(this); // should be `Owner`, not `| undefined`
      }
    }

Presently, we return `Owner | undefined` instead.
Use `GlimmerComponent<any>` instead of plain `GlimmerComponent`, since
the latter resolves to the default `GlimmerComponent<unknown>`, which
ends up being too narrow to resolve correctly with `getOwner` when any
actual signature is applied.

As a bonus, rename `FrameworkObject` in this context to the somewhat
more verbose `KnownFrameworkObject`, which avoids a conflict with the
actual `FrameworkObject` type used internally.
The type for `getOwner()` is *technically* unsafe, but this unsafety is
one which end users will rarely hit in practice and, courtesy of Ember's
current types, is unavoidable if `getOwner(someService)` is to correctly
return `Owner` instead of `Owner | undefined`.

The Typed Ember folks have discussed this point at some length, but I
realized on looking at this to make the changes in the preceding two
commits that we didn't have it committed to the code base anywhere, so
here I am just introducing a long comment explaining it. When we switch
from preview to stable types, this comment should come along!
// 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.

@chriskrycho chriskrycho merged commit 698674b into master Nov 8, 2022
@chriskrycho chriskrycho deleted the fix-types-FrameworkObject branch November 8, 2022 14:49
@chriskrycho chriskrycho changed the title [BUGFIX release] Fix types for getOwner and GlimmerComponent [BUGFIX release] Fix types for getOwner and GlimmerComponent Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants