feat(ivy): implement the getters of ViewContainerRef #25174

vicb · 2018-07-27T22:58:25Z

supersedes #25092

(Rebased after recent changes to di.ts)

~~edit 7/29: added a second commit for misc refactoring + fix CI (had to revert a change I did on Friday as import * as viewEngine breaks tree shaking).~~

edit 7/30: As noted by @marclaval below, it was using Injector.NULL not import * as viewEngine that was breaking tree shaking. Updated the 2nd commit

googlebot · 2018-07-27T22:58:28Z

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 the commit author(s) and merge this pull request when appropriate.

vicb · 2018-07-27T23:02:10Z

packages/core/src/render3/di.ts

@mhevery @marclaval
vcr.element returns a different instance each time and also !== vcr.injector.get(ElementRef)
is this the expected behavior ?

mary-poppins · 2018-07-27T23:06:24Z

You can preview a89b3b3 at https://pr25174-a89b3b3.ngbuilds.io/.

googlebot · 2018-07-27T23:06:47Z

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

vicb · 2018-07-27T23:07:39Z

@marclaval @mhevery The original PR was green and good to go but I added a question before this can be merged. Feel free to add the "merge, ..." labels if this is not a problem. Thanks.

vicb · 2018-07-27T23:12:21Z

packages/core/src/render3/instructions.ts

nit: viewData ? viewData[INJECTOR] : null

vicb · 2018-07-27T23:13:22Z

packages/core/test/render3/view_container_ref_spec.ts

nit: use element() when Start and End are back to back to conform to what the compiler generates.
(there is a PR pending for updating current specs)

googlebot · 2018-07-29T21:35:28Z

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 the commit author(s) and merge this pull request when appropriate.

mary-poppins · 2018-07-29T21:43:22Z

You can preview c763810 at https://pr25174-c763810.ngbuilds.io/.

marclaval · 2018-07-30T07:51:56Z

@vicb about your question:
vcr.element returns a different instance each time and also !== vcr.injector.get(ElementRef)
is this the expected behavior ?

I don't know why but this is how it is done in the current engine: https://github.com/angular/angular/blob/master/packages/core/src/view/refs.ts#L137
I've kept the same behaviour.

marclaval · 2018-07-30T08:07:21Z

@vicb: using Injector.NULL instead of new NullInjector() brings in a bunch of new tokens in the todo app bundle: https://circleci.com/gh/angular/angular/59905
It doesn't seem to be such a good idea.

vicb · 2018-07-30T15:04:14Z

using Injector.NUL instead of new NullInjector() ...

Thanks I was tracking this one. I will revert.

Wrt to a new instance of ElementRef, I'll discuss with Misko and we can update this PR accordingly so that it can be merged ASAP.

Thanks

mary-poppins · 2018-07-30T15:20:26Z

You can preview 8091d1e at https://pr25174-8091d1e.ngbuilds.io/.

mary-poppins · 2018-07-30T15:29:47Z

You can preview ea0dded at https://pr25174-ea0dded.ngbuilds.io/.

mary-poppins · 2018-07-30T15:33:38Z

You can preview 8543f3d at https://pr25174-8543f3d.ngbuilds.io/.

googlebot · 2018-07-30T17:50:52Z

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

vicb · 2018-07-30T17:52:47Z

http://test/OCL:206389457:BASE:206609379:1532973094314:5c898f9b

mary-poppins · 2018-07-30T17:58:56Z

You can preview 25f790b at https://pr25174-25f790b.ngbuilds.io/.

marclaval · 2018-07-30T18:12:33Z

LGTM

IgorMinar

just rubber-stamping to make CI happy - the original PR from Marc had all the approvals

BREAKING CHANGE: ViewContainerRef.parentInjector is deprecated without replacement

googlebot · 2018-07-30T23:56:32Z

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 the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-30T23:57:14Z

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

PR Close #25174

mary-poppins · 2018-07-31T00:04:03Z

You can preview 1d86b37 at https://pr25174-1d86b37.ngbuilds.io/.

angular-automatic-lock-bot · 2019-09-13T22:21:58Z

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

googlebot added the cla: no label Jul 27, 2018

vicb commented Jul 27, 2018

View reviewed changes

vicb mentioned this pull request Jul 27, 2018

feat(ivy): implement the getters of ViewContainerRef #25092

Closed

vicb added cla: yes action: review The PR is still awaiting reviews from at least one requested reviewer and removed cla: no labels Jul 27, 2018

vicb commented Jul 27, 2018

View reviewed changes

packages/core/src/render3/instructions.ts Outdated

Copy link

Contributor Author

vicb Jul 27, 2018

Choose a reason for hiding this comment

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

nit: viewData ? viewData[INJECTOR] : null

vicb commented Jul 27, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Jul 29, 2018

vicb added target: major This PR is targeted for the next major release comp: ivy labels Jul 29, 2018

vicb force-pushed the gh-25092 branch 2 times, most recently from 8091d1e to ea0dded Compare July 30, 2018 15:19

vicb force-pushed the gh-25092 branch from ea0dded to 8543f3d Compare July 30, 2018 15:23

vicb added cla: yes and removed cla: no labels Jul 30, 2018

IgorMinar requested a review from mhevery July 30, 2018 21:05

IgorMinar approved these changes Jul 30, 2018

View reviewed changes

vicb removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 30, 2018

IgorMinar removed the request for review from mhevery July 30, 2018 21:23

marclaval and others added 3 commits July 30, 2018 16:54

feat(ivy): implement the getters of ViewContainerRef

7ff8014

BREAKING CHANGE: ViewContainerRef.parentInjector is deprecated without replacement

refactor(ivy): misc

ca37df5

fixup! refactor(ivy): misc

1d86b37

vicb force-pushed the gh-25092 branch from 25f790b to 1d86b37 Compare July 30, 2018 23:56

googlebot added cla: no and removed cla: yes labels Jul 30, 2018

vicb added cla: yes and removed cla: no labels Jul 30, 2018

IgorMinar closed this in cd89eb8 Jul 31, 2018

IgorMinar pushed a commit that referenced this pull request Jul 31, 2018

refactor(ivy): misc (#25174)

3664829

PR Close #25174

vicb deleted the gh-25092 branch August 3, 2018 19:08

edusperoni mentioned this pull request Jun 18, 2019

Migrate from ReflectiveInjector (deprecated) to StaticInjector NativeScript/nativescript-angular#1868

Merged

5 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

feat(ivy): implement the getters of ViewContainerRef #25174

feat(ivy): implement the getters of ViewContainerRef #25174

Uh oh!

Conversation

vicb commented Jul 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jul 27, 2018

Uh oh!

vicb Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Jul 27, 2018

Uh oh!

googlebot commented Jul 27, 2018

Uh oh!

vicb commented Jul 27, 2018

Uh oh!

vicb Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

vicb Jul 27, 2018

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 29, 2018

Uh oh!

mary-poppins commented Jul 29, 2018

Uh oh!

marclaval commented Jul 30, 2018

Uh oh!

marclaval commented Jul 30, 2018

Uh oh!

vicb commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mary-poppins commented Jul 30, 2018

Uh oh!

mary-poppins commented Jul 30, 2018

Uh oh!

mary-poppins commented Jul 30, 2018

Uh oh!

googlebot commented Jul 30, 2018

Uh oh!

vicb commented Jul 30, 2018

Uh oh!

mary-poppins commented Jul 30, 2018

Uh oh!

marclaval commented Jul 30, 2018

Uh oh!

IgorMinar left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jul 30, 2018

Uh oh!

googlebot commented Jul 30, 2018

Uh oh!

mary-poppins commented Jul 31, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vicb commented Jul 27, 2018 •

edited

Loading

vicb commented Jul 30, 2018 •

edited

Loading