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(ivy): support @SkipSelf while injecting special DI tokens #34839

Closed

Conversation

AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jan 17, 2020

Currently Ivy doesn't support adding @SkipSelf for some of the special DI tokens (e.g. Injector, ViewContainerRef, ElementRef, ChangeDetectorRef). These changes add support for @SkipSelf flag and align the behavior with View Engine.

Closes #34066.
Closes #34819.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix target: patch This PR is targeted for the next patch release comp: ivy labels Jan 17, 2020
@ngbot ngbot bot modified the milestone: needsTriage Jan 17, 2020
@AndrewKushnir
Copy link
Contributor Author

Related to #34554.

@AndrewKushnir AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 21, 2020
@AndrewKushnir AndrewKushnir marked this pull request as ready for review January 21, 2020 18:10
@AndrewKushnir AndrewKushnir requested review from a team as code owners January 21, 2020 18:10
@AndrewKushnir AndrewKushnir requested a review from a team January 21, 2020 18:10
Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 24, 2020
@IgorMinar
Copy link
Contributor

merge-assistance: global approval

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: presubmit The PR is in need of a google3 presubmit labels Jan 24, 2020
@AndrewKushnir AndrewKushnir force-pushed the skip_self_injector branch 2 times, most recently from 02651eb to a0832d4 Compare January 25, 2020 00:39
@AndrewKushnir
Copy link
Contributor Author

Presubmit

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jan 27, 2020
@AndrewKushnir AndrewKushnir added cla: yes action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 22, 2020
@AndrewKushnir
Copy link
Contributor Author

Presubmit

@AndrewKushnir
Copy link
Contributor Author

@kara thanks for the review!

I'd like to ask you to have a look at this PR again, since I had to update a bunch of payload limits, which I believe for the most part is an accumulation of payload size increase introduced in other commits merged to master prior to this change, see #34839 (comment).

Thank you.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AndrewKushnir
Copy link
Contributor Author

Quick update: investigation of g3 failures revealed a couple use-cases that work incorrectly with the changes in this PR due to the tView structure implementation that doesn't allow to easily go up the chain (in case it's a cross-boundary read). In order to make further progress, some updates to tView structure is required. As a pre-requisite to that work, it'd be helpful to land PR #34715 to minimize the number of affected lines of code and avoid dealing with unnecessary tests. I'll clean up #34715 and request a review shortly. Thank you.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@angular angular deleted a comment from googlebot Feb 26, 2020
@googlebot
Copy link

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.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 3, 2020
@AndrewKushnir AndrewKushnir added cla: yes state: WIP and removed action: presubmit The PR is in need of a google3 presubmit state: blocked cla: no labels Mar 3, 2020
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@pkozlowski-opensource pkozlowski-opensource added the area: core Issues related to the framework runtime label May 9, 2020
@AndrewKushnir
Copy link
Contributor Author

Closing this PR in favor of #39464.

@angular-automatic-lock-bot
Copy link

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 Nov 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime cla: yes state: WIP target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
6 participants