Use typed get method in TestBed class

[michaeljota]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[x] 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

The get static method of TestBed class does not return a typed object.

const service = TestBed.get(MyService); // service is typed as any

Expected behavior

The get static method should return a typed object.

const service = TestBed.get(MyService); // service should be typed as MyService

Minimal reproduction of the problem with instructions

Look examples.

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

This would improve type safety.

Environment

Angular version: 6.1.9

Others:

A possible implementation using new helper types from TypeScript would be:

    static get<T extends new (...args: any[]) => any>(token: T, notFoundValue?: T | InstanceType<T>): InstanceType<T>;

I'm not sure if the notFoundValue should be a constructor or an instance, so I typed it as both, but fell free to update it as is actually needed. I tested that signature and it actually returns a typed object as wanted.

[trotyl]

Relates to #18695, #23611, #24151

[harshabada]

did u added the above service in providers

[mhevery]

A PR would be great!

[michaeljota]

@mhegazy I'll try to get into it. No promises.

[Goodwine]

@michaeljota hiya, I was facing this same issue today and found your report, are you working on a PR? otherwise I'm thinking I can try to send one, just didn't want to step over

[michaeljota]

Hi @Goodwine. I haven't get a chance to look at this. You can give it a look if you want. Thanks!

[Goodwine]

My proposal is to use these types -

get<T>(token: InjectionToken<T>): T;
get<T>(token: Type<T>): T;
get(token: T): unknown;
get<T1, T2>(token: InjectionToken<T1>, notFoundValue: T2): T1|T2;
get<T1, T2>(token: Type<T1>, notFoundValue: T2): T1|T2;
get<T2>(token: T, notFoundValue: T2): unknown;

The reason for using unknown when the value isn't either an InjectionToken or a Type is because the desired type is actually an abstract class, and this will force the user to cast to the correct value.

There is one problem with this approach though. What if the user wants to use the abstract class interface? Or what if the user knows the real value? Unfortunately this is kinda annoying to do in TS because if we have an abstract class Foo, the return value would be typeof Foo. We're only able to get away with it with concrete classes because angular has this fancy type Type which extends anything with a constructor so for a concrete class Bar, the inferred value is Type<Bar> which allows for easily returning of type Bar rather than typeof Bar

Perhaps the return value should be never so that the user isn't even allowed to cast the return value and instead they would have to cast the argument. For example:

// bad?
  const foo = TestBed.get(Foo); // returns "never"
// good?
  const foo = TestBed.get(Foo as Type<Foo>) // returns type Foo

Problem is that this could be anything and the user could cast to whatever they want, so maybe returning unknown is best?

[michaeljota]

I would consider an approach that reduce type casting, as I think is just about that.

[michaeljota]

@Goodwine What do you think about something like this:

    get<T>(token: T | InjectionToken<T>, notFoundValue?: T): T  extends new (...args: any[]) => any ? InstanceType<T> : T;

I think this would reduce type casting. I'm want to be sure that I understand the usage of Type<T> in your examples, but I think this can be done with conditional typing.

[Goodwine]

Type<T> is a type that essentially maps to new (...args: any[]) => any, it's already in @angular/core.

I was pretty sure there was a way to get T out of typeof T but I didn't know how. InstanceType basically solves everything now! 😃

One thing I'm still not sure what to do about.

get<T1, T2>(token: T1, notFoundValue: T2): T1|T2;

Should this be T1|T2? Or just T1? My thoughts are that there should only be one type. T.
However in Angular TestBed code there is a bug where the values are boolean[] and boolean respectively. So I think I'm starting to lean towards not having T1 and T2, and just having T and have angular deal with it.

[Goodwine]

Hm.. sorry for the early celebration, it seems it wouldn't work with InstanceType because that one only works on classes. Casting is still a must.

http://www.typescriptlang.org/play/#src=class%20Foo%20%7B%20%0D%0A%20%20%20%20callFoo()%20%7B%7D%0D%0A%7D%0D%0Aabstract%20class%20Bar%20%7B%0D%0A%20%20%20%20callBar()%20%7B%7D%0D%0A%20%7D%0D%0A%0D%0A%2F%2F%20Type%3CT%3E%20defined%20in%20%40angular%2Fcore%0D%0Aexport%20interface%20Type%3CT%3E%20extends%20Function%20%7B%20new%20(...args%3A%20any%5B%5D)%3A%20T%3B%20%7D%0D%0A%0D%0Afunction%20get%3CT%3E(token%3A%20Type%3CT%3E%2C%20notFoundValue%3F%3A%20T)%3A%20T%3B%0D%0A%2F%2F%20fails%20with%3A%0D%0A%2F%2F%20Type%20'T'%20does%20not%20satisfy%20the%20constraint%20'new%20(...args%3A%20any%5B%5D)%20%3D%3E%20any'.%0D%0A%2F%2F%20T%20is%20not%20a%20class.%0D%0Afunction%20get%3CT%3E(token%3A%20T%2C%20notFoundValue%3F%3A%20T)%3A%20InstanceType%3CT%3E%3B%0D%0Afunction%20get%3CT%3E(k%3A%20T)%3A%20T%7B%0D%0A%20%20%20%20return%20%22%22%20as%20any%3B%20%2F%2F%20ignore%20this%20line.%0D%0A%7D%0D%0A%0D%0A%2F%2F%20This%20seems%20to%20work%2C%20but%20does%20it%20really%20work%3F%0D%0Afunction%20get2%3CT%3E(token%3A%20T%2C%20notFoundValue%3F%3A%20T)%3A%20T%20extends%20Type%3Cany%3E%20%3F%20InstanceType%3CT%3E%20%3A%20T%3B%0D%0Afunction%20get2%3CT%3E(k%3A%20T)%3A%20T%7B%0D%0A%20%20%20%20return%20%22%22%20as%20any%3B%20%2F%2F%20ignore%20this%20line.%0D%0A%7D%0D%0A%0D%0Aconst%20foo%20%3D%20get2(Foo)%3B%0D%0Afoo.callFoo()%3B%0D%0A%0D%0Aconst%20bar%20%3D%20get2(Bar)%3B%0D%0A%2F%2F%20Fails%20with%3A%0D%0A%2F%2F%20Property%20'callBar'%20does%20not%20exist%20on%20type%20'typeof%20Bar'.%0D%0Abar.callBar()%3B%0D%0A%0D%0A%2F%2F%20Casting%20works%20tho%0D%0Aconst%20bar2%20%3D%20get2(Bar%20as%20Type%3CBar%3E)%3B%0D%0Abar2.callBar()%3B

[michaeljota]

Strictly speaking, new (...args: any[]) => any it's different from Type<T>. With the first, you are saying that the value should be a constructor, or have a constructor shape, but with the second you are saying that it should be a constructor function, and should return T. I guess it's because conditional typing is new in Typescript, and Type<T> has been around for before that.

In any case, I don't think there is an advantage using Type<T>. What do you think? Maybe get another review on this.

About the usage of an abstract class as a token, not even with Type<T> it would be doable:

Link to playground Lost work :(

Maybe return any in the other side of the condition, but this would reduce the chance of using InjectionToken<T> and return T. Maybe if you could find a condition to know if the value is an InjectionToken, you could return T, or if neither a constructor or an InjectionToken, then return any. What do you think?

[Goodwine]

I can apparently use infer as a magic word within extends and create an AbstractType type 😮

type AbstractType<T> = T extends { prototype: infer T } ? T : never;

Resulting in:

get<T>(token: InjectionToken<T>): T;
get<T>(token: T): T extends Type<unknown> ? InstanceType<T> : AbstractType<T>;
get<T1, T2>(token: InjectionToken<T1>, notFoundValue: T2): T1 | T2;
get<T1, T2>(token: T1, notFoundValue: T2): T1 extends Type<unknown> ? InstanceType<T1>|T2 : AbstractType<T1>|T2;

I really disagree with using any, however I think this should be good enough. Thoughts?

[michaeljota]

I don't is necessary that many overloads. And I'm not sure about AbstractType.

Check this:

Playground link

baz is resolved to have type never. IMO this is undesirable. I would rather have any as I have it now, than never, as I would need to cast it.

[Goodwine]

The reason it's not working in your example is because your get() function has the right types, but you are using get2() with the wrong types.

Link

I'm thinking that it's possible to have:

get(x: T): T extends InjectionToken<any> ? T : T extends Type<any> ? T : AbstractType<T>;

But I feel like it's too annoying to read and reason about.

Regarding why having overload for whether the second argument is present, because I'm using <T1, T2> the return time would otherwise be T1|undefined. I'm guessing I could add something like...

get(x: T, y?: T2): (T extends InjectionToken<any> ? T : T extends Type<any> ? T : AbstractType<T>)|(T2 extends undefined ? never : T2);

But I have the same concern of this being too hard to read.

[Goodwine]

(reason for using <T1, T2> is that angular uses something like get(NgZone, null) which should return NgZone|null but would otherwise return NgZone

[michaeljota]

If you don't supply an additional argument, it would return null, but if you are using it, that argument should be the same shape as T.

What Angular does underneath shouldn't be a concern, but instead it should be to express better the intention of the function.

However, I don't know if there is value in returning T | null in testing scenarios, as then you should either a) supply an additional shape, undesired. b) use ! operator, also undesired.

BTW: In deed, I test with get function an it does work as expected. I still think that it have many overloads and they could be merged, but it works great so far.

[michaeljota]

@Goodwine I still not sure about T1 | T2.

If you want to get something, say Foo, and want something in return if that is not found, that something should be the same shape as Foo, doesn't it?

[Goodwine]

So the reason I chose to use 2 types is because you may want to do get(Foo, null) and you'd expect Foo|null but in order to do that with a single type you need to do get(Foo as typeof Foo|null, null) or get(Foo as Type<Foo>|null)

I can't think of other reason and to be honest I've never seen any other uses except for within Angular's code base, but that's not to say they are implementation details, as far as I'm aware this is all part of the public API.

There are also "options" that can be passed to get() as a second argument, they are new Object() so they need to be casted. I'm not familiar with their use, but with what I could gather, two examples are "return undefined if not found" and "throw if undefined or not found".

Otherwise I agree with you, I'd much rather have only 1 type in there. But to be fair, if you add something on the second parameter that has a different shape, the type would protect you now, and if you add a mock/spy, and now that I think even more about it, I wonder why do we even have that option for TestBed given that in a test you probably shouldn't be using the second parameter. Hm...

[michaeljota]

Why would you want to get null if not found? I'm trying to understand that scenario.

[Goodwine]

That's just to support the existing use of get(NgZone, null), I'd rather have one type as mentioned above.

To be honest I'd like to get rid of the second param for return this when the other was not found, a variadic parameter for option (the way it's currently used) would be OK with me tho.

[Goodwine]

Talking with the angular team (I'm not part of the angular team), I'll actually send PRs in the following order:

1. Add @deprecate to current function signature and add same signature as Injector.get, however it would still return any.
   • This already happened on fix(di): add better type information to injector.get() #13785 but now breaking changes aren't as easy to make :)
2. Add set of types that will eventually be used, even if they aren't used yet.
3. Insert breaking changes that could land in a future major version, but definitely not Angular 8

[Goodwine]

Per discussion with @vikerman, the Angular team can't accept such a breaking change at this point, so the approach we'll now take is to create a new method TestBed.inject which will be type-safe. And deprecate TestBed.get.

[angular-automatic-lock-bot]

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