Invalid inject() function typing

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

inject()'s type description seems to be invalid:

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

Lines 78 to 80 in 5cfde8b

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

Return type should be T | null only if flag is set to InjectFlags.Optional meaning that the signature should be something like this:

 export function ɵɵinject<T>(token: ProviderToken<T>): T; 
 export function ɵɵinject<T>(token: ProviderToken<T>, flags?: InjectFlags.Default | InjectFlags...): T; 
 export function ɵɵinject<T>(token: ProviderToken<T>, flags?: InjectFlags.Optional): T | null; 
 export function ɵɵinject<T>(token: ProviderToken<T>, flags = InjectFlags.Default): T|null

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

No response

Please provide the environment you discovered this bug in (run `ng version`)

No response

Anything else?

No response

This would be a feature request for a narrower return type of inject, as the current signatures are not invalid. They just don't make a distinction between various injection flags.

Since InjectFlags is a bit mask, this requires special care. The following set of signature overloads makes this work as desired:

declare function inject<T>(token: Type<T>): T;
declare function inject<T>(token: Type<T>, flags: InjectFlags.Optional): T|null;
declare function inject<T>(token: Type<T>, flags: InjectFlags.Default | InjectFlags.Host | InjectFlags.Self | InjectFlags.SkipSelf): T;
declare function inject<T>(token: Type<T>, flags: InjectFlags): T|null;

TypeScript playground

What is concerning here though is that TypeScript always selects the second overload (the one with InjectFlags.Optional) for inline bit masks, as can be seen in the skipSelfAndSelf case in the playground. This is unlike using a variable of type InjectFlags, which does resolve to the last overload as desired.

Currently this sort of works out, especially in practice as it doesn't make sense to combine any of the Self, SkipSelf and Host flags into a single call. It does concern me though that TypeScript doesn't select the last overload for skipSelfAndSelf, as it indicates that TypeScript's overload resolution isn't equipped for handling this properly at the moment. It also means that the specific ordering of overload 2 and 3 is important, which ideally it should not be (the signatures don't have overlap in their flags parameter, so their relative ordering shouldn't affect overload resolution but it does).

This feature request is now candidate for our backlog! In the next phase, the community has 60 days to upvote. If the request receives more than 20 upvotes, we'll move it to our consideration list.

You can find more details about the feature request process in our documentation.

@JoostK

Thanks @JoostK for diving so deeply into this & also taking care of the repro 😅.
I've been playing with it and the TypeScript resolution behavior is so intriguing.
It simply seems that bitwise operators resolve to number which is compatible with any InjectFlags value, that's why it picks the first signature with a flag parameter:

Meanwhile, this gets fixed in TypeScript, I think that the best solution is the one you came out with.
It reduces the use cases where users will have to cast the return type to flag combinations.

Do you want me to go ahead with a PR?

Thanks for your time

A PR is welcome, that would allow for a TGP (running all g3 tests) to see what the impact of such a change would be.

I'm taking care of it.

I think the right answer here is deprecate the current inject format with the bitfield options and add a new one which uses an options object instead.

@yjaaidi

@yjaaidi happy that you agree with that, I hope you didn't spend too much time already on the types changes

The suggestion to create a new issue was based on the fact that it the change would be more than just improving the typing but it's an actual api change, so I was thinking that it could be worth it to open a specific issue for that, especially to formalize the issue and also get suggestions (especially from the angular team), I think it would be a bit clearer instead of continuing the conversation here (but here may be fine too I'd imagine)

Your changes do look great to me :D
(but I would definitely wait for someone else who knows this functionality well to check them, as @JoostK or @AndrewKushnir)

Another thing, it may be a bit of a silly one, but if we are going to replace the flags with an object could we maybe rename self to something like onlySelf, untilSelf or similar? I think self is a bit less clear, or is that just me? 😛

@yjaaidi

Quick update: we've discussed this topic with the team and we want to move forward with a PR that would:

add the support for an object as the second argument in the inject and Injector.get functions (the Injector.get should be a separate commit), while keeping the support for the current format as well for backwards compatibility
add the deprecation annotation to the type signatures where an old format is used, with a recommendation to use a new object-based format

One more thing thing to consider is the difference between the inject and Injector.get signatures: the Injector.get has an extra argument to provide a default value if none was found in the DI. Ideally, it'd be great to fully align inject and Injector.get, but it would be a separate effort.

@yjaaidi please let us know if you might be interested in creating a PR to add support for the options object.

@dario-piotrowicz

Another thing, it may be a bit of a silly one, but if we are going to replace the flags with an object could we maybe rename self to something like onlySelf, untilSelf or similar? I think self is a bit less clear, or is that just me?

@dario-piotrowicz good question. I think it'd be valuable to keep the mapping between the current flags (which also map to the decorators like @Self, @Host, etc) and the inject flags, so it's easier for developers to transition to the options object. However, we should also consider this improvement (may be we can add extra flags, which would be just aliases) and we can have an additional discussion to decide on the final set of flags once the draft PR is ready.

@yjaaidi

@yjaaidi I would invert the types (example)

declare type Token<T> = {new(): T};

export interface InjectOptions {
  optional?: boolean;
  self?: boolean;
  skipSelf?: boolean;
  host?: boolean;
}

class TestToken {}

declare function inject<T>(type: Token<T>, flags?: InjectOptions&{optional?: false}): T;
declare function inject<T>(type: Token<T>, flags: InjectOptions): T|null;

let value: TestToken;

value = inject(TestToken);
value = inject(TestToken, {self: true});
// Error: `null` is a potential return value:
value = inject(TestToken, {optional: true});

@alxhub

Thank you so much for taking care of this @alxhub and sorry again for not responding faster with a PR

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

Navigation Menu

Repository navigation

Invalid `inject()` function typing #46251

Which @angular/* package(s) are the source of the bug?

Is this a regression?

Description

Please provide a link to a minimal reproduction of the bug

Please provide the exception or error you saw

Please provide the environment you discovered this bug in (run `ng version`)

Anything else?

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Invalid inject() function typing #46251

Description

Which @angular/* package(s) are the source of the bug?

Is this a regression?

Description

Please provide a link to a minimal reproduction of the bug

Please provide the exception or error you saw

Please provide the environment you discovered this bug in (run ng version)

Anything else?

Activity

JoostK commented on Jun 4, 2022

angular-robot commented on Jun 4, 2022

yjaaidi commented on Jun 4, 2022

JoostK commented on Jun 4, 2022

yjaaidi commented on Jun 4, 2022

alxhub commented on Jun 6, 2022

4 remaining items

dario-piotrowicz commented on Jun 9, 2022

dario-piotrowicz commented on Jun 9, 2022

AndrewKushnir commented on Jun 9, 2022

AndrewKushnir commented on Jun 9, 2022

alxhub commented on Jun 13, 2022

yjaaidi commented on Jul 4, 2022

angular-automatic-lock-bot commented on Aug 4, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Invalid `inject()` function typing #46251

Please provide the environment you discovered this bug in (run `ng version`)