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

[FEATURE] Add TypeScript support for looking up controllers in DI registry #20521

Merged

Conversation

littleredridingfox
Copy link

This PR enables controller type discovery through Owner::lookup method:

declare module '@ember/controller' {
  interface Registry {
    foo: FooController;
  }
}

const fooController = this.owner.lookup('controller:foo'); // TS resolves fooController's type as FooController.
const barController = this.owner.lookup('controller:bar'); // If not specified via Registry interface, will resolve type as Controller | undefined

Complementary PR to @types/ember__controller: DefinitelyTyped/DefinitelyTyped#66067

@chriskrycho
Copy link
Contributor

We'll discuss, and I expect we'll merge this, since we have the corresponding APIs covered elsewhere. I want to say, however: I didn't implement it for a reason, which is that folks should absolutely move away from using controller lookup like this, for a host of reasons, most of all that controller lifecycle is weird and that 99% of things people do with controllers should be done with routes, services, and components instead. (The only real exception is query param management!) Like I said, though: since the API is there, I expect we will likely support it.

While we are discussing, folks should be aware that you can implement this yourself in any app or addon without needing official support. (This is the other part of the reason I did not prioritize implementing it when adding general DI registry support for lookup.)

@chriskrycho chriskrycho changed the title [BUGFIX stable] @ember/controller's Registry is not registered in @ember/owner's DIRegistry [FEATURE] Add TypeScript support for looking up controllers in DI registry Aug 11, 2023
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

I'm making one tweak to the docs and then we can land this; note that I'm also tweaking what it lands as—a feature, not a bug fix. The previous types were correct here—they would never have caused a runtime error—so this is best flagged as a new capability.

Do please consider migrating away from controller injections to using service injections or passing arguments instead, as that is much more aligned with where the framework is headed!

packages/@ember/controller/index.ts Outdated Show resolved Hide resolved
@chriskrycho chriskrycho merged commit a28a285 into emberjs:main Aug 11, 2023
15 checks passed
@chriskrycho chriskrycho added enhancement TypeScript Work on Ember’s types labels Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement TypeScript Work on Ember’s types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants