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

Tree Shakable Providers and InjectionToken: inject() does not work with abstract classes #23611

Closed
manfredsteyer opened this issue Apr 30, 2018 · 11 comments
Labels
area: core Issues related to the framework runtime core: di freq1: low P4 A relatively minor issue that is not relevant to core functions type: bug/fix
Milestone

Comments

@manfredsteyer
Copy link
Contributor

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ x ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

You cannot use the new inject with abstract classes, as it expects an InjectionToken or a Type which is defined as "new-able":

export const DEMMO_SERVICE = new InjectionToken<DemoService>('DEMO_SERVICE', {
  providedIn: 'root',
  // this doesn't work as HeroService is abstract
  factory: () => new DemoService(inject(HeroService))
})

Expected behavior

It should be possible to use inject with abstract classes.

Sidenote: inject from @angular/core/testing uses any instead and this includes abstract classes too.

Minimal reproduction of the problem with instructions

Please see comments in
https://stackblitz.com/edit/angular-yxr5ny-foth3r?file=src%2Fapp%2Fapp.tokens.ts

What is the motivation / use case for changing the behavior?

  • API symmetrie
  • going with the principle of the least surprise
  • the current behavior could be a show stopper in some situations

Environment


Angular version: 6.0.0-rc.6
Tested with Stackblitz (see above)
@trotyl
Copy link
Contributor

trotyl commented Apr 30, 2018

Relates to microsoft/TypeScript#17572, there's no way to type-check an abstract class.

@manfredsteyer
Copy link
Contributor Author

Yes, but we could go with any. I know, this is not a pretty solution but it fits to Angular b/c a token can technically be of any type (also see https://angular.io/api/core/Inject).

@trotyl
Copy link
Contributor

trotyl commented May 1, 2018

Not actually, Symbols are not allowed due to technically restriction, and there could be more restrictions to go as #15319 (comment).

@manfredsteyer
Copy link
Contributor Author

I see. That means, we have two options.

  1. Beeing too restrictive which means we cannot use abstract classes
  2. Beeing too tolerant which means the type system allows abstract classes but also other things we are not happy with

None of them is perfect. But when we consider that it is very common to use abstract classes as tokens, I think option 2 is the better one. Also, there are many cases where Angular follows this pattern and it was possible in the past, which means that option 1 would break things we are used to.

@trotyl
Copy link
Contributor

trotyl commented May 1, 2018

it was possible in the past

Not since v4, it already won't work in current Injector API if you don't ignore the deprecated warning.

abstract get<T>(token: Type<T>|InjectionToken<T>, notFoundValue?: T, flags?: InjectFlags): T;
/**
* @deprecated from v4.0.0 use Type<T> or InjectionToken<T>
* @suppress {duplicate}
*/
abstract get(token: any, notFoundValue?: any): any;

Whatever deprecated in v4 should be removed in v6.

So this request is NOT about the new inject() method introduced in v6, but the existing DI system.

@manfredsteyer
Copy link
Contributor Author

I see. Thanks for pointing this out. Frankly, this is not ideal, since using abstract base types for such things is in general considert good design.

@IgorMinar IgorMinar added the area: core Issues related to the framework runtime label May 3, 2018
@ngbot ngbot bot added this to the needsTriage milestone May 3, 2018
@ngbot ngbot bot modified the milestones: needsTriage, Backlog May 3, 2018
trotyl added a commit to trotyl/angular that referenced this issue Jul 31, 2018
trotyl added a commit to trotyl/angular that referenced this issue Jul 31, 2018
trotyl added a commit to trotyl/angular that referenced this issue Jul 31, 2018
mhevery pushed a commit to trotyl/angular that referenced this issue Sep 23, 2018
mhevery pushed a commit to trotyl/angular that referenced this issue Oct 2, 2018
mhevery pushed a commit to trotyl/angular that referenced this issue Oct 3, 2018
@rgant
Copy link
Contributor

rgant commented May 29, 2019

Trying to resolve my difficulties with OnPush change detection in unit tests and I needed to get the ChangeDetectorRef of my Component to trigger change detection. But that caused these warnings because ChangeDetectorRef is an abstract class.

@trotyl
Copy link
Contributor

trotyl commented May 30, 2019

Trying to resolve my difficulties with OnPush change detection in unit tests and I needed to get the ChangeDetectorRef of my Component to trigger change detection.

@rgant That's #29905, you don't use this inject() API in testing.

@probert94
Copy link

We just enabled the "deprecation"-rule of tslint and I noticed a few of them in our app.
I "fixed" them using a cast to Type:
this.injector.get(<Type<HeroService>>HeroService)

@jelbourn jelbourn added P4 A relatively minor issue that is not relevant to core functions and removed severity2: inconvenient labels Oct 1, 2020
@JoostK
Copy link
Member

JoostK commented May 13, 2021

Support for abstract classes as tokens was added in #37958, so closing this as resolved.

@JoostK JoostK closed this as completed May 13, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: di freq1: low P4 A relatively minor issue that is not relevant to core functions type: bug/fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants