fix(core): Access injected parent values using SkipSelf #39464

jessicajaniuk · 2020-10-27T23:15:46Z

In ViewEngine, SelfSkip would navigate up the tree to get tokens from
the parent node, skipping the child. This restores that functionality in
Ivy. In ViewEngine, if a special token (e.g. ElementRef) was not found
in the NodeInjector tree, the ModuleInjector was also used to lookup
that token. While special tokens like ElementRef make sense only in a
context of a NodeInjector, we preserved ViewEngine logic for now to
avoid breaking changes.

We identified 4 scenarios related to @SkipSelf and special tokens where
ViewEngine behavior was incorrect and is likely due to bugs. In Ivy this is
implemented to provide a more intuitive API. The list of scenarios can be
found below.

When Injector is used in combination with @Host and @SkipSelf
on the first Component within a module and the injector is defined in the
module, ViewEngine will get the injector from the module. In Ivy, it does
not do this and throws instead. Test
When retrieving a @ViewContainerRef while @SkipSelf and @Host
are present, in ViewEngine, it throws an exception. In Ivy it returns the host
ViewContainerRef. Test
When retrieving a @ViewContainerRef on an embedded view and
@SkipSelf is present, in ViewEngine, the ref is null. In Ivy it returns the
parent ViewContainerRef. Test
When utilizing viewProviders and providers, a child component that is
nested within a parent component that has @SkipSelf on a viewProvider
value, if that provider is provided by the parent component's viewProviders
and providers, ViewEngine will return that parent's viewProviders value,
which violates how viewProviders' visibility should work. In Ivy, it retrieves
the value from providers, as it should. Test

These discrepancies all behave as they should in Ivy and are likely bugs in
ViewEngine.

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?

Currently Ivy ignores @SkipSelf entirely, which deviates from ViewEngine and expectations of behavior.

Issue Number: 34066, 34819

What is the new behavior?

@SkipSelf now properly navigates up the view tree to find the correct injected resource in the parents.

Does this PR introduce a breaking change?

Yes
No

Closes #34066, #34819.

AndrewKushnir

Great work @jessicajaniuk 👍 I've left a few comments.

packages/core/src/render3/di.ts

packages/core/test/acceptance/di_spec.ts

packages/core/src/render3/di.ts

packages/core/test/acceptance/di_spec.ts

petebacondarwin

Love all those new tests! Just left a few drive-by comments that you can ignore if you wish.

packages/core/src/render3/di.ts

packages/core/test/acceptance/di_spec.ts

JoostK

There is a typo in the commit message, which mentions SelfSkip instead of SkipSelf.

packages/core/src/render3/di.ts

google-cla · 2020-11-05T21:49:52Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-11-05T22:14:59Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

AndrewKushnir · 2020-11-05T22:49:08Z

Presubmit.

AndrewKushnir

Reviewed-for: size-tracking

atscott

Reviewed-for: size-tracking

@SkipSelf

In ViewEngine, SelfSkip would navigate up the tree to get tokens from the parent node, skipping the child. This restores that functionality in Ivy. In ViewEngine, if a special token (e.g. ElementRef) was not found in the NodeInjector tree, the ModuleInjector was also used to lookup that token. While special tokens like ElementRef make sense only in a context of a NodeInjector, we preserved ViewEngine logic for now to avoid breaking changes. We identified 4 scenarios related to @SkipSelf and special tokens where ViewEngine behavior was incorrect and is likely due to bugs. In Ivy this is implemented to provide a more intuitive API. The list of scenarios can be found below. 1. When Injector is used in combination with @host and @SkipSelf on the first Component within a module and the injector is defined in the module, ViewEngine will get the injector from the module. In Ivy, it does not do this and throws instead. 2. When retrieving a @ViewContainerRef while @SkipSelf and @host are present, in ViewEngine, it throws an exception. In Ivy it returns the host ViewContainerRef. 3. When retrieving a @ViewContainerRef on an embedded view and @SkipSelf is present, in ViewEngine, the ref is null. In Ivy it returns the parent ViewContainerRef. 4. When utilizing viewProviders and providers, a child component that is nested within a parent component that has @SkipSelf on a viewProvider value, if that provider is provided by the parent component's viewProviders and providers, ViewEngine will return that parent's viewProviders value, which violates how viewProviders' visibility should work. In Ivy, it retrieves the value from providers, as it should. These discrepancies all behave as they should in Ivy and are likely bugs in ViewEngine. PR Close #39464

angular-automatic-lock-bot · 2020-12-07T16:33:27Z

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

google-cla bot added the cla: yes label Oct 27, 2020

jessicajaniuk added the area: core Issues related to the framework runtime label Oct 27, 2020

ngbot bot added this to the needsTriage milestone Oct 27, 2020

AndrewKushnir mentioned this pull request Oct 28, 2020

fix(ivy): support @SkipSelf while injecting special DI tokens #34839

Closed

3 tasks

jessicajaniuk force-pushed the skipself branch from 16ae2ad to 7a80a16 Compare October 28, 2020 17:01

AndrewKushnir reviewed Oct 28, 2020

View reviewed changes

JoostK mentioned this pull request Oct 28, 2020

@SkipSelf() cd: ChangeDetectorRef doesn't return parent ChangeDetector #39475

Closed

jessicajaniuk force-pushed the skipself branch 2 times, most recently from 9720620 to c88b046 Compare October 28, 2020 20:22

AndrewKushnir reviewed Oct 28, 2020

View reviewed changes

jessicajaniuk force-pushed the skipself branch 4 times, most recently from f54a2fc to 7d0ddaa Compare October 29, 2020 20:50

jessicajaniuk added the target: patch This PR is targeted for the next patch release label Oct 29, 2020

jessicajaniuk marked this pull request as ready for review October 29, 2020 20:55

jessicajaniuk force-pushed the skipself branch from 7d0ddaa to bcdce27 Compare October 29, 2020 23:21

jessicajaniuk added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 29, 2020

AndrewKushnir reviewed Oct 30, 2020

View reviewed changes

jessicajaniuk force-pushed the skipself branch 4 times, most recently from f460802 to 14fe03f Compare November 2, 2020 19:36

AndrewKushnir reviewed Nov 2, 2020

View reviewed changes

jessicajaniuk force-pushed the skipself branch 4 times, most recently from 71471fe to d5abfdf Compare November 3, 2020 01:39

petebacondarwin approved these changes Nov 3, 2020

View reviewed changes

JoostK reviewed Nov 3, 2020

View reviewed changes

packages/core/src/render3/di.ts Outdated Show resolved Hide resolved

packages/core/src/render3/di.ts Outdated Show resolved Hide resolved

packages/core/src/render3/di.ts Outdated Show resolved Hide resolved

google-cla bot added cla: no and removed cla: yes labels Nov 5, 2020

jessicajaniuk added cla: yes and removed cla: no labels Nov 5, 2020

fixup! fix(core): Access injected parent values using SelfSkip

6a6c475

jessicajaniuk force-pushed the skipself branch from 760b6b1 to 6a6c475 Compare November 5, 2020 22:14

google-cla bot added cla: no and removed cla: yes labels Nov 5, 2020

jessicajaniuk added cla: yes and removed cla: no labels Nov 5, 2020

AndrewKushnir approved these changes Nov 5, 2020

View reviewed changes

pullapprove bot requested a review from alxhub November 5, 2020 22:50

atscott approved these changes Nov 6, 2020

View reviewed changes

AndrewKushnir removed request for petebacondarwin, jelbourn and alxhub November 6, 2020 00:13

AndrewKushnir added target: rc This PR is targeted for the next release-candidate and removed target: minor This PR is targeted for the next minor release action: presubmit The PR is in need of a google3 presubmit labels Nov 6, 2020

jessicajaniuk added the action: merge The PR is ready for merge by the caretaker label Nov 6, 2020

mhevery closed this in 290ea57 Nov 6, 2020

This was referenced Nov 6, 2020

In Ivy, SkipSelf() is not accessing the parent's ViewContainerRef, ElementRef, etc #34066

Closed

SkipSelf annotation to get parent component Injector no longer working in Ivy (Angular 9 rc.9) #34819

Closed

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

jessicajaniuk deleted the skipself branch October 26, 2021 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Access injected parent values using SkipSelf #39464

fix(core): Access injected parent values using SkipSelf #39464

jessicajaniuk commented Oct 27, 2020 •

edited

AndrewKushnir left a comment

petebacondarwin left a comment

JoostK left a comment

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

AndrewKushnir commented Nov 5, 2020

AndrewKushnir left a comment

atscott left a comment

angular-automatic-lock-bot bot commented Dec 7, 2020

fix(core): Access injected parent values using SkipSelf #39464

fix(core): Access injected parent values using SkipSelf #39464

Conversation

jessicajaniuk commented Oct 27, 2020 • edited

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

AndrewKushnir left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

google-cla bot commented Nov 5, 2020

google-cla bot commented Nov 5, 2020

AndrewKushnir commented Nov 5, 2020

AndrewKushnir left a comment

Choose a reason for hiding this comment

atscott left a comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Dec 7, 2020

jessicajaniuk commented Oct 27, 2020 •

edited