-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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): allow root components to inject ViewContainerRef #26682
fix(ivy): allow root components to inject ViewContainerRef #26682
Conversation
You can preview e4743d9 at https://pr26682-e4743d9.ngbuilds.io/. |
e4743d9
to
84d3062
Compare
You can preview 84d3062 at https://pr26682-84d3062.ngbuilds.io/. |
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 |
@@ -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;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that you have to cast tells me that our ObjectOrientedRenderer3
does not match document
* views into. | ||
*/ | ||
[RENDER_PARENT]: RElement|null; | ||
[RENDER_PARENT]: RNode|null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can parent be something other than RElement
?
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can you remove from something other than RElement
?
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.) |
You can preview 3e3a61e at https://pr26682-3e3a61e.ngbuilds.io/. |
3e3a61e
to
b3e0649
Compare
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 |
b3e0649
to
6b28813
Compare
CLAs look good, thanks! |
You can preview 6b28813 at https://pr26682-6b28813.ngbuilds.io/. |
6b28813
to
079664f
Compare
You can preview 079664f at https://pr26682-079664f.ngbuilds.io/. |
eedfc66
to
a4d433b
Compare
You can preview a4d433b at https://pr26682-a4d433b.ngbuilds.io/. |
a4d433b
to
c70da1e
Compare
You can preview c70da1e at https://pr26682-c70da1e.ngbuilds.io/. |
c70da1e
to
31fc453
Compare
You can preview 31fc453 at https://pr26682-31fc453.ngbuilds.io/. |
Marking as merge-assistance since:
|
205dba9
to
7cd961d
Compare
You can preview 7cd961d at https://pr26682-7cd961d.ngbuilds.io/. |
89223b0
to
c448a09
Compare
You can preview c448a09 at https://pr26682-c448a09.ngbuilds.io/. |
cc6981f
to
8e5374c
Compare
You can preview 8e5374c at https://pr26682-8e5374c.ngbuilds.io/. |
merge-assistance since g3presubmit is needed. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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
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).