fix(ivy): allow root components to inject ViewContainerRef #26682

pkozlowski-opensource · 2018-10-23T14:37:06Z

This PR introduces introduces ability to inject ViewContainerRef on the root / topmost / bootstrapped component. This is supported in the current view engine and used in some tests (ex.:

angular/packages/common/src/directives/ng_component_outlet.ts

Line 85 in 6737e91

constructor(private _viewContainerRef: ViewContainerRef) {}

).

In case of ViewContainerRef injected on the topmost component we need to do low-level DOM manipulation to find parent of the host node (to insert a comment node) and a sibling of the host node (to place a comment node next to it). Since both the comment node and its parent will be outside of LTree we need to resort to DOM manipulation.

I'm not super-happy about this change and open to discussing alternatives (where alternative could be simply banning ability to inject ViewContainerRef into the topmost component).

mary-poppins · 2018-10-23T14:42:07Z

You can preview e4743d9 at https://pr26682-e4743d9.ngbuilds.io/.

mary-poppins · 2018-10-23T14:54:02Z

You can preview 84d3062 at https://pr26682-84d3062.ngbuilds.io/.

googlebot · 2018-10-23T18:11:55Z

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.

mhevery · 2018-10-23T17:21:49Z

packages/core/src/render3/interfaces/renderer.ts

@@ -94,11 +97,15 @@ export interface RendererFactory3 {

 export const domRendererFactory3: RendererFactory3 = {
  createRenderer: (hostElement: RElement | null, rendererType: RendererType2 | null):
-                      Renderer3 => { return document;}
+                      Renderer3 => { return document as ObjectOrientedRenderer3;}


The fact that you have to cast tells me that our ObjectOrientedRenderer3 does not match document

mhevery · 2018-10-23T17:22:03Z

packages/core/src/render3/interfaces/container.ts

   * views into.
   */
-  [RENDER_PARENT]: RElement|null;
+  [RENDER_PARENT]: RNode|null;


How can parent be something other than RElement?

mhevery · 2018-10-23T17:22:22Z

packages/core/src/render3/interfaces/renderer.ts

@@ -68,9 +68,12 @@ export interface ProceduralRenderer3 {
  destroyNode?: ((node: RNode) => void)|null;
  appendChild(parent: RElement, newChild: RNode): void;
  insertBefore(parent: RNode, newChild: RNode, refChild: RNode|null): void;
-  removeChild(parent: RElement, oldChild: RNode): void;
+  removeChild(parent: RNode, oldChild: RNode): void;


How can you remove from something other than RElement?

googlebot · 2018-10-23T18:12:29Z

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

mary-poppins · 2018-10-23T18:15:43Z

You can preview 3e3a61e at https://pr26682-3e3a61e.ngbuilds.io/.

googlebot · 2018-10-24T08:19:00Z

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 · 2018-10-24T10:01:58Z

CLAs look good, thanks!

mary-poppins · 2018-10-24T10:05:55Z

You can preview 6b28813 at https://pr26682-6b28813.ngbuilds.io/.

mary-poppins · 2018-10-24T12:28:07Z

You can preview 079664f at https://pr26682-079664f.ngbuilds.io/.

mary-poppins · 2018-10-25T08:27:35Z

You can preview a4d433b at https://pr26682-a4d433b.ngbuilds.io/.

mary-poppins · 2018-10-25T08:49:26Z

You can preview c70da1e at https://pr26682-c70da1e.ngbuilds.io/.

mhevery · 2018-10-25T22:56:05Z

http://test/OCL:218765714:BASE:218765822:1540508154645:77f652dd

mary-poppins · 2018-10-26T08:24:48Z

You can preview 31fc453 at https://pr26682-31fc453.ngbuilds.io/.

ngbot · 2018-10-26T12:17:34Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "ci/circleci: test_docs_examples_0" is failing
status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

pkozlowski-opensource · 2018-10-26T12:18:26Z

Marking as merge-assistance since:

ci/circleci: test_docs_examples_0 is failing for unrelated reasons
google3 pre-submit is pending and I've got no control over it

mary-poppins · 2018-10-29T09:21:25Z

You can preview 7cd961d at https://pr26682-7cd961d.ngbuilds.io/.

mary-poppins · 2018-10-29T09:37:44Z

You can preview c448a09 at https://pr26682-c448a09.ngbuilds.io/.

mary-poppins · 2018-10-29T10:01:51Z

You can preview 8e5374c at https://pr26682-8e5374c.ngbuilds.io/.

pkozlowski-opensource · 2018-10-29T16:58:03Z

merge-assistance since g3presubmit is needed.

kara · 2018-10-29T17:02:18Z

presubmit

…6682) PR Close angular#26682

angular-automatic-lock-bot · 2019-09-14T06:22:34Z

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: yes label Oct 23, 2018

pkozlowski-opensource added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Oct 23, 2018

ngbot bot added this to the needsTriage milestone Oct 23, 2018

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from e4743d9 to 84d3062 Compare October 23, 2018 14:49

googlebot added cla: no and removed cla: yes labels Oct 23, 2018

mhevery approved these changes Oct 23, 2018

View reviewed changes

mhevery added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes and removed cla: no labels Oct 23, 2018

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from 3e3a61e to b3e0649 Compare October 24, 2018 08:18

googlebot added cla: no and removed cla: yes labels Oct 24, 2018

pkozlowski-opensource removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 24, 2018

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from b3e0649 to 6b28813 Compare October 24, 2018 10:01

googlebot added cla: yes and removed cla: no labels Oct 24, 2018

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from 6b28813 to 079664f Compare October 24, 2018 12:23

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from eedfc66 to a4d433b Compare October 25, 2018 08:23

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from a4d433b to c70da1e Compare October 25, 2018 08:45

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch from c70da1e to 31fc453 Compare October 26, 2018 08:21

pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Oct 26, 2018

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch 3 times, most recently from 205dba9 to 7cd961d Compare October 29, 2018 09:18

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch 3 times, most recently from 89223b0 to c448a09 Compare October 29, 2018 09:34

pkozlowski-opensource force-pushed the ivy_vcref_on_root_component branch 2 times, most recently from cc6981f to 8e5374c Compare October 29, 2018 09:58

fix(ivy): allow root components to inject ViewContainerRef

8e5374c

kara 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 Oct 29, 2018

matsko closed this in ede65db Oct 29, 2018

sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018

fix(ivy): allow root components to inject ViewContainerRef (angular#2…

2776c60

…6682) PR Close angular#26682

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019

fix(ivy): allow root components to inject ViewContainerRef (angular#2…

71106ba

…6682) PR Close angular#26682

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): allow root components to inject ViewContainerRef #26682

fix(ivy): allow root components to inject ViewContainerRef #26682

pkozlowski-opensource commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

googlebot commented Oct 23, 2018

mhevery Oct 23, 2018

mhevery Oct 23, 2018

mhevery Oct 23, 2018

googlebot commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

googlebot commented Oct 24, 2018

googlebot commented Oct 24, 2018

mary-poppins commented Oct 24, 2018

mary-poppins commented Oct 24, 2018

mary-poppins commented Oct 25, 2018

mary-poppins commented Oct 25, 2018

mhevery commented Oct 25, 2018

mary-poppins commented Oct 26, 2018

ngbot bot commented Oct 26, 2018

pkozlowski-opensource commented Oct 26, 2018

mary-poppins commented Oct 29, 2018

mary-poppins commented Oct 29, 2018

mary-poppins commented Oct 29, 2018

pkozlowski-opensource commented Oct 29, 2018

kara commented Oct 29, 2018

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

fix(ivy): allow root components to inject ViewContainerRef #26682

fix(ivy): allow root components to inject ViewContainerRef #26682

Conversation

pkozlowski-opensource commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

googlebot commented Oct 23, 2018

mhevery Oct 23, 2018

Choose a reason for hiding this comment

mhevery Oct 23, 2018

Choose a reason for hiding this comment

mhevery Oct 23, 2018

Choose a reason for hiding this comment

googlebot commented Oct 23, 2018

mary-poppins commented Oct 23, 2018

googlebot commented Oct 24, 2018

googlebot commented Oct 24, 2018

mary-poppins commented Oct 24, 2018

mary-poppins commented Oct 24, 2018

mary-poppins commented Oct 25, 2018

mary-poppins commented Oct 25, 2018

mhevery commented Oct 25, 2018

mary-poppins commented Oct 26, 2018

ngbot bot commented Oct 26, 2018

pkozlowski-opensource commented Oct 26, 2018

mary-poppins commented Oct 29, 2018

mary-poppins commented Oct 29, 2018

mary-poppins commented Oct 29, 2018

pkozlowski-opensource commented Oct 29, 2018

kara commented Oct 29, 2018

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