fix(core): support InjectFlags argument in NodeInjector.get()

[vthinkxie]

close #31776

the InectFlags was defined in both getOrCreateInjectable and Injector, but was ignored in NodeInjector
this PR support getting parent token via injector.get in NodeInjector

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?

• 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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• Yes
• No

Other information

[AndrewKushnir]

@vthinkxie thanks for creating this PR. A couple comments:

• This change should also contain a number of tests that would make sure the flags work as expected and are aligned with token injection performed via constructor arguments.
• The NodeInjector is a part of the Ivy logic and the ViewEngine version of it doesn't seem to have the flags support either (see here). We'd need to consider adding the logic there too (for general feature parity).

We'll discuss this with the team more and I will followup here with some additional information.

Thank you.

[AndrewKushnir]

Hi @vthinkxie, sorry for the delay in answering.

We've discussed this feature request with the team and we'd like to move forward with the implementation. A couple notes:

• Passing flags from NodeInjector.get -> getOrCreateInjectable makes sense.
• This change requires a number of tests that would make sure the flags work as expected and are aligned with token injection performed via constructor arguments. You can add more tests in the packages/core/test/acceptance/di_spec.ts file.
• Since ViewEngine is deprecated, the change can be isolated to Ivy code only (i.e. what you've already changed).

Please let me know if any additional information is required (on adding tests or anything else related to this change).

Thank you.

[vthinkxie]

Hi @AndrewKushnir

I have added some tests in packages/core/test/acceptance/di_spec.ts, could you have a look? thanks

[vthinkxie]

Hi @AndrewKushnir
thanks for your review, I have updated the code, could you take a look?

[AndrewKushnir]

Hi @vthinkxie, thanks for addressing the feedback. I will have a look at the changes within the next couple days and let you know.

One thing that I noticed is that the ci/circleci: components-repo-unit-tests CI job is failing. I've looked at the failure and found the code in Components repo that relies on the Injector.get flags, see here. My guess is that the mentioned code assumes that the 3rd argument is supported (when it's actually ignored) and the change that you made in this PR makes use of that argument, thus changing the result. We will look into this more and let you know.

Thank you.

[vthinkxie]

Thanks, @AndrewKushnir
hope this PR will be in time for the v12 release

[petebacondarwin]

Oh there is a typo in the commit message. Can we update it to:

fix(core): support `InjectFlags` argument in `NodeInjector.get()`

The `InjectFlags` argument was defined for both `getOrCreateInjectable()` and `Injector`, but was ignored in `NodeInjector`.
This PR adds support for getting parent token via `NodeInjector.get()`.

Closes #31776

[vthinkxie]

Oh there is a typo in the commit message. Can we update it to:

fix(core): support `InjectFlags` argument in `NodeInjector.get()`

The `InjectFlags` argument was defined for both `getOrCreateInjectable()` and `Injector`, but was ignored in `NodeInjector`.
This PR adds support for getting parent token via `NodeInjector.get()`.

Closes #31776

done

[AndrewKushnir]

@vthinkxie thanks for working on this change 👍

[AndrewKushnir]

Presubmit.

[AndrewKushnir]

Quick update after running tests in Google's codebase: there is a failing target with the changes from this PR. The underlying reason is that the 3rd argument was provided (the flags), but the code used to ignore it and with this change, the correct logic is now invoked. The situation is similar to the one we have in the Components repo (where the injector.get is used with flags, assuming NodeInjector there). The code in Google's codebase can be cleaned up, but I wanted to ask if Components team can help with the assessment of whether the mentioned code in the Components repo may become an issue (may start failing in some situations after we merge this change) and if we should preemptively remove the InjectFlags.Self (since it should have no effect right now).

// cc @crisbeto @jelbourn

[crisbeto]

The linked code from the Components repo should still work. We added the | InjectFlags.Optional specifically to unblock this PR.

[petebacondarwin]

@crisbeto:

Tthe concern was more that InjectFlags.Self was not being passed through before, and now will be. So previously it would have had no effect and now it will. If this makes no difference then is there a reason to have that flag there at all?

Also, I don't understand why this code would fail after this PR but not before? Does passing undefined equate to passing InjectFlags.Optional?

[crisbeto]

I think that we want to keep the InjectFlags.Self there, because we only want the control from the date range input itself. I suspect that this worked fine before, because there was always a control on the same level so the injector didn't have to look up the NgControl in a parent.

Regarding the optional flag, my understanding was that if it isn't passed, the injector will throw an error if it can't find the token, whereas with the flag it returns the fallback value.

[AndrewKushnir]

@crisbeto thanks for the comment. I will proceed with the cleanup in Google's codebase and provide an update once we can proceed with the merge (hopefully within the next couple days).

[AndrewKushnir]

Global Presubmit #2 (internal-only link).

[AndrewKushnir]

FYI presubmits went well. @petebacondarwin please let us know if you have any additional feedback and if not, we can proceed with the merge. Thank you.

[petebacondarwin]

Let's merge it!

[alxhub]

This PR was merged into the repository by commit 65cb2c5.

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