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

fix(core): make injector.get() return default value with InjectFlags.… #27739

Closed
wants to merge 1 commit into from

Conversation

@thekiba
Copy link
Contributor

commented Dec 18, 2018

…Self flag on

Fixes #27729

PR Checklist

Please check if your PR fulfills the following requirements:

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: #27729

What is the new behavior?

injector.get() return default value with InjectFlags.Self flag on

Does this PR introduce a breaking change?

  • Yes
  • No

injector.get() will return default value with InjectFlags.Self flag on instead of undefined

Other information

@googlebot googlebot added the cla: yes label Dec 18, 2018
@ngbot ngbot bot added this to the needsTriage milestone Dec 19, 2018
@googlebot

This comment has been minimized.

Copy link

commented Dec 19, 2018

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 googlebot added cla: no and removed cla: yes labels Dec 19, 2018
@thekiba

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@pkozlowski-opensource I don't know what googlebot wants. It is waiting for confirmation or isn't?

@JoostK

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

@thekiba googlebot can't verify the CLA as this PR now contains commits from various authors. This is typically not a problem and will be resolved by a Googler (by hand).

Anyway, the second commit that contains my suggestion can be folded into the initial commit. We'll want for this change to only be a single commit anyway.

Locally, please pull the changes from this branch and get creative with Git. You can rebase, but the easiest way for me to explain here would be to softly reset to your commit (HEAD~) and amend my changes (then in the index because of the soft reset) to the first commit:

git pull
git reset --soft HEAD~
git commit --amend
git push --force-with-lease

That should do it.

@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch from 07cd1af to 955abb9 Dec 20, 2018
@googlebot

This comment has been minimized.

Copy link

commented Dec 20, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 20, 2018
@mhevery mhevery self-assigned this Jan 15, 2019
@mhevery mhevery force-pushed the thekiba:bugfix/27729 branch from 955abb9 to bbbe1d8 Jan 15, 2019
@thekiba thekiba requested a review from angular/fw-core as a code owner Jan 15, 2019
@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch from 59f7f0b to 53116cf Mar 28, 2019

it('should return a default value when not requested provider on self', () => {
const injector = Injector.create([]);
expect(injector.get<Car|null>(Car, null, InjectFlags.Self)).toBeNull();

This comment has been minimized.

Copy link
@mhevery

mhevery Apr 9, 2019

Member

The above implementation is not quite right If you add additional tests.

expect(() => injector.get<Car|null>(Car, undefined, InjectFlags.Self)).toThrow('....');

You will see that it will not pass. Could you fix it?

This comment has been minimized.

Copy link
@thekiba

thekiba Apr 10, 2019

Author Contributor

Did it! 🙌

Also I added some more tests. For example, should return null when there is no default value and the Self and Optional flag on.

@@ -336,6 +336,8 @@ function resolveToken(
}
} else if (!(flags & InjectFlags.Self)) {
value = parent.get(token, notFoundValue, InjectFlags.Default);
} else {
value = notFoundValue;

This comment has been minimized.

Copy link
@mhevery

mhevery Apr 9, 2019

Member

I think this should delegate to NullInjector.get so that there is a chance to throw a proper error.

This comment has been minimized.

Copy link
@thekiba

thekiba Apr 10, 2019

Author Contributor

I delegated it to NullInjector and added additional check for Optional flag

@mhevery

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Also pleases rebase on the latest master and squash commits into single commit.

@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch 2 times, most recently from aba84fa to 765d3e3 Apr 10, 2019
@mhevery mhevery force-pushed the thekiba:bugfix/27729 branch from 765d3e3 to dbf3c1c Jul 26, 2019
@mhevery

This comment has been minimized.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

@thekiba thanks for the fix!

It looks like the tests are failing because NULL_INJECTOR was not imported. Could you please have a look?

Thank you.

@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch 2 times, most recently from 8244549 to 3870018 Jul 30, 2019
@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch from 3870018 to 4bf15da Jul 30, 2019
@thekiba

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@AndrewKushnir I have replaced NULL_INJECTOR to Injector.NULL and fixed it.

@mhevery @AndrewKushnir I found a bug with Ivy R3Injector, it doesn't return a default value when not requested provider on self and optional flags. I fixed it too.

And I found another one with Ivy. R3Injector.get(), NgModuleRef.get(), etc, sets notFoundValue as THROW_IF_NOT_FOUND when it undefined. In this case, we cannot return the expected undefined value, because it sets as THROW_IF_NOT_FOUND. We can see it by the failed test.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@thekiba thanks for additional fixes. It looks like test_ivy_aot is failing with injector-related error, which might potentially be related to your changes. Could you please have a look?

We may also need to ask @mhevery to have a quick look at the changes once again when the mentioned failure is fixes, since you added some extra code to r3_injector as you pointed out.

Thank you.

@thekiba thekiba force-pushed the thekiba:bugfix/27729 branch 3 times, most recently from 6410b7f to 1ca540a Jul 31, 2019
@thekiba

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@AndrewKushnir thanks! test_ivy_aot is failing because I added extra test for static_injector_spec.ts witch successfully passed for not Ivy injector. I added a fix for it: notFoundValue checks as THROW_IF_NOT_FOUND instead of undefined.

Also, I have a few problems with Bazel and gulp locally. Can we discuss that somewhere?

@AndrewKushnir AndrewKushnir requested a review from mhevery Jul 31, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@mhevery could you please have another look at the changes in this PR (specifically around r3_injector.ts)? Thank you.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

@thekiba thanks for additional fixes!

I think you can open a ticket using "Issues" section and describe the problem there (with a repro if possible), so the team and community members can have a look and try to help.

Thank you.

@thekiba

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I'm still having this pr

@AndrewKushnir AndrewKushnir force-pushed the thekiba:bugfix/27729 branch from 1ca540a to eb98682 Sep 12, 2019
@atscott

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2019

Update:

  • Blueprint and Global TAP Presubmits for both VE and Ivy are successful
  • CI is green
  • Misko approved this PR
  • I've changed the label to "master-only" (so this change would be a part of v9 release), since this is still a slight behavior change

Based on this, I'm adding "merge" label back.

Thank you.

@kara kara closed this in 0477bfc Sep 13, 2019
arnehoek added a commit to arnehoek/angular that referenced this pull request Sep 26, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Oct 14, 2019

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 Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.