fix(core): allow abstract class as injection token by trotyl · Pull Request #25222 · angular/angular

trotyl · 2018-07-31T13:50:39Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #23611

There're many services used an abstract class as tokens, like:

After #13785 merged, injector.get(ChangeDetectorRef) would produce a deprecation warning since an abstract class is not a Type:

angular/packages/core/src/di/injector.ts

Lines 71 to 75 in 3f20a2f

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

Also in the new inject() API, it throws an error as no deprecated any fallback:

angular/packages/core/src/di/injector.ts

Lines 459 to 461 in 3f20a2f

    
           export function inject<T>(token: Type<T>| InjectionToken<T>): T; 
        
           export function inject<T>(token: Type<T>| InjectionToken<T>, flags?: InjectFlags): T|null; 
        
           export function inject<T>(token: Type<T>| InjectionToken<T>, flags = InjectFlags.Default): T|null {

What is the new behavior?

Abstract classes should be allowed as injection tokens, this PR has relaxed typing of Type so that abstract class can be now used.

Technically any non-class function can also pass the type now as they have an any prototype.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

If one uses Type type and trying to new it directly.

Other information

JoostK · 2018-07-31T14:18:48Z

Instead of relaxing the constraints of Type, we could also introduce a new type for Token in general. That would avoid the cast to Constructor for e.g. useClass providers. Furthermore, providers would not be able to incorrectly use an abstract class as value type (useClass or using the shortcut syntax of only the class), only as token (the provide property would be of type Token, same as inject's parameter).

trotyl · 2018-07-31T14:38:26Z

@JoostK With providedIn usage, the type-to-construct is determined at consumer-side, not provider-side:

@Injectable({
  providedIn: 'root'
})
abstract class Foo { }

The newable or not of class Foo cannot be combined with Injectable options (extra provider or not), so there's no validation in the provider-side and consumer-side can only allow an abstract class without validation.

Then it will always be casting as newing an abstract class cannot be prevented.

mhevery · 2018-08-07T22:36:41Z

What is the reasoning to change null to undefined?

angular/packages/core/src/linker/component_factory.ts

Lines 85 to 87 in 732026c

abstract create(

injector: Injector, projectableNodes?: any[][], rootSelectorOrNode?: string|any,

ngModule?: NgModuleRef<any>): ComponentRef<C>;

ComponentFactory#create() accepts NgModuleRef | undefined, not NgModuleRef | null, it's a compilation error in TypeScript.

Previously ngModule is of any type so bypassed the type-checking.

mhevery · 2018-08-07T22:48:18Z

I think this is a breaking change since we are changing the meaning of Type. I would much rather create AbstractType which is than extended by Type

interface AbstractType<T> extends Function { prototype: T; } interface Type<T> extends AbstractType<T> { new (...args: any[]): T;}

I think that would work better and be less breaking. Many of the changes you had to do would not need to be made. What do you think?

Sounds reasonable, I'll have a try.

This is indeed less breaking, but would let Type being completely meaningless, every API would only use AbstractType rather than Type.

_{AbstractType | Type is just AbstractType.}

googlebot · 2018-09-23T03:43:59Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-09-23T03:46:51Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery

Created fixup for outstanding issues.

mhevery · 2018-09-23T03:48:11Z

http://test/OCL:214144130:BASE:214144163:1537674480964:6ddc23f4

googlebot · 2018-09-23T05:35:06Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-09-23T05:38:35Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery · 2018-09-23T05:39:23Z

http://test/OCL:214144130:BASE:214148814:1537681151554:eb68748d

mhevery · 2018-09-23T19:30:43Z

There are a lot of breaking targets in Googl3 from this PR so this seems to be a breaking change. Needs more investigation.

javascript/angular2/testing/catalyst/helpers.ts:765:12 - error TS2352: Type 'typeof DynamicTemplateComponent' cannot be converted to type 'Type<T>'.
  Types of property 'prototype' are incompatible.
    Type 'DynamicTemplateComponent' is not comparable to type 'T'.

765     return DynamicTemplateComponent as Type<T>;

trotyl · 2018-09-24T03:00:21Z

@mhevery The code return DynamicTemplateComponent as Type<T>; doesn't make sense, how would a non-generic concrete type be coerced to an opened generic type where T is defined by caller?

googlebot · 2018-10-02T05:06:16Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-10-03T04:47:33Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

mhevery · 2018-10-03T04:52:28Z

http://test/OCL:214144130:BASE:215512383:1538542333897:5e488cea
http://test/OCL:214144130:BASE:215513796:1538543544767:e4074f2d
http://test/OCL:214144130:BASE:215573657:1538584151154:6a94bcd6
http://test/OCL:214144130:BASE:215576931:1538585308716:4978f811

IgorMinar

I like this version of the PR, but I don't see any test that proves that abstract classes can actually be used as injection tokens. Can you please add one?

IgorMinar · 2018-10-03T15:39:21Z

The PR also needs a rebase, which might uncover other compilation issues (many tests didn't run on CI). And I see that google3 has quite a few compilation failures as well.

The combination of all of these makes me more and more uncomfortable about landing this change now. If we can't make this a no/low developer pain change, we should wait with this until v8 and make a plan to roll it out.

closes angular#23611

googlebot · 2018-10-03T16:41:50Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

mhevery · 2018-10-03T16:47:16Z

Rebased and added a test.

mhevery · 2018-10-03T17:08:59Z

Issues:

`injector.get()`

  /**
   * Retrieves an instance from the injector based on the provided token.
   * @returns The instance from the injector if defined, otherwise the `notFoundValue`.
   * @throws When the `notFoundValue` is `undefined` or `Injector.THROW_IF_NOT_FOUND`.
   */
  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;

Notice that when an abstract type would be feed into get it would came back as any because the it would not match the Type. When we fix the get to accept AbstractType than we go form any to T on the return type this. breaks many call sites where developers are incorrectly taking advantage of any.

CDK wants `ConcreteType`.

Material2 CDK defines ComponentType as ConcreteType (which is correct because only concrete types can be dynamically loaded)

/** Interface that can be used to generically type a class. */
export interface ComponentType<T> {
  new (...args: any[]): T;
}

The issue is that if Angular changes Type to both AbstractType as well as ConcreteType it causes a type error since CDK's type is more restrictive than Angular's type.

Required Work To Land PR

Remove the deprecated method abstract get(token: any, notFoundValue?: any): any; from the Injector
- Document the breaking change
- Need automated refactoring in g3 to migrate
- Create a schematic to automate migration for our users.
CDK ConcreteType
- Document the breaking change as to why Type no longer can be assigned to ComponentType
- Need automated refactoring in g3 to migrate
- Create a schematic to automate migration for our users.

Conclusion

It is non-trivial amount of work and it is too late to land this for v7 and we will defer this for v8.

AndrewKushnir · 2020-11-18T00:59:12Z

Hi, I'm closing this PR in favor of recently active #37958 that targets Ivy code. Thank you.

angular-automatic-lock-bot · 2020-12-19T16:39:14Z

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.}

googlebot added the cla: yes label Jul 31, 2018

trotyl force-pushed the fix-type-abstract-class branch from 45bbce3 to 86c6692 Compare July 31, 2018 14:07

IgorMinar requested a review from mhevery August 1, 2018 21:31

IgorMinar added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime labels Aug 1, 2018

mhevery suggested changes Aug 7, 2018

View reviewed changes

JoostK mentioned this pull request Sep 22, 2018

fix(ivy): objects like ElementRef should not use a special injection fn #26064

Closed

mhevery force-pushed the fix-type-abstract-class branch from 86c6692 to f0bba3a Compare September 23, 2018 03:43

googlebot added cla: no and removed cla: yes labels Sep 23, 2018

mhevery added cla: yes and removed cla: no labels Sep 23, 2018

mhevery approved these changes Sep 23, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Sep 23, 2018

mhevery added cla: yes and removed cla: no labels Sep 23, 2018

mhevery force-pushed the fix-type-abstract-class branch from b3fc729 to 3cdfda7 Compare October 2, 2018 05:06

googlebot removed the cla: yes label Oct 2, 2018

IgorMinar suggested changes Oct 3, 2018

View reviewed changes

trotyl and others added 8 commits October 3, 2018 09:33

fix(core): allow abstract class as injection token

8da2c34

closes angular#23611

fixup! fix(core): allow abstract class as injection token

12e9682

fixup! fix(core): allow abstract class as injection token

c84e579

fixup! fix(core): allow abstract class as injection token

1e24ba1

fixup! fix(core): allow abstract class as injection token

98ff9f7

fixup! fix(core): allow abstract class as injection token

0f88516

fixup! fix(core): allow abstract class as injection token

b8a3e4e

fixup! fix(core): allow abstract class as injection token

9045583

mhevery force-pushed the fix-type-abstract-class branch from 575ee45 to 9045583 Compare October 3, 2018 16:41

googlebot added cla: no and removed cla: yes labels Oct 3, 2018

manughub added this to the v8-candidates milestone Oct 8, 2018

manughub added the breaking changes label Oct 8, 2018

JoostK mentioned this pull request Mar 18, 2019

feat(core): Add "AbstractType<T>" interface #29295

Closed

5 of 14 tasks

trotyl mentioned this pull request May 30, 2019

HttpClient testing guide suggests using deprecated TestBed.get invocation on abstract class HttpTestingController #30750

Closed

mpalourdio mentioned this pull request May 31, 2019

Remove workaround for deprecation warnings in TestBed mpalourdio/ng-http-loader#119

Closed

IgorMinar modified the milestones: v9-candidates, v10-candidates Nov 1, 2019

AndrewKushnir closed this Nov 18, 2020

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 19, 2020

pullapprove Bot added the area: compiler Issues related to `ngc`, Angular's template compiler label Dec 19, 2020

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

	export function inject<T>(token: Type<T>\| InjectionToken<T>): T;
	export function inject<T>(token: Type<T>\| InjectionToken<T>, flags?: InjectFlags): T\|null;
	export function inject<T>(token: Type<T>\| InjectionToken<T>, flags = InjectFlags.Default): T\|null {

	abstract create(
	injector: Injector, projectableNodes?: any[][], rootSelectorOrNode?: string\|any,
	ngModule?: NgModuleRef<any>): ComponentRef<C>;

Conversation

trotyl commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

JoostK commented Jul 31, 2018

Uh oh!

trotyl commented Jul 31, 2018

Uh oh!

mhevery Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

trotyl Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

mhevery Aug 7, 2018

Choose a reason for hiding this comment

Uh oh!

trotyl Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

trotyl Aug 8, 2018

Choose a reason for hiding this comment

Uh oh!

googlebot commented Sep 23, 2018

Uh oh!

googlebot commented Sep 23, 2018

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Sep 23, 2018

Uh oh!

googlebot commented Sep 23, 2018

Uh oh!

googlebot commented Sep 23, 2018

Uh oh!

mhevery commented Sep 23, 2018

Uh oh!

mhevery commented Sep 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trotyl commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Oct 2, 2018

Uh oh!

googlebot commented Oct 3, 2018

Uh oh!

mhevery commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Oct 3, 2018

Uh oh!

googlebot commented Oct 3, 2018

Uh oh!

mhevery commented Oct 3, 2018

Uh oh!

mhevery commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues:

injector.get()

CDK wants ConcreteType.

Required Work To Land PR

Conclusion

Uh oh!

AndrewKushnir commented Nov 18, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Dec 19, 2020

Uh oh!

Reviewers

Uh oh!

trotyl commented Jul 31, 2018 •

edited

Loading

mhevery commented Sep 23, 2018 •

edited

Loading

trotyl commented Sep 24, 2018 •

edited

Loading

mhevery commented Oct 3, 2018 •

edited

Loading

mhevery commented Oct 3, 2018 •

edited

Loading

`injector.get()`

CDK wants `ConcreteType`.