refactor(ivy): use comment nodes to mark view containers #24346
Conversation
marclaval
added
action: review
|
You can preview 63bb48c at https://pr24346-63bb48c.ngbuilds.io/. |
|
You can preview 3b70063 at https://pr24346-3b70063.ngbuilds.io/. |
packages/core/src/render3/di.ts
Outdated
| const lContainerNode: LContainerNode = createLNodeObject( | ||
| TNodeType.Container, vcRefHost.view, hostParent, undefined, lContainer, null); | ||
| TNodeType.Container, vcRefHost.view, hostParent, comment, lContainer, null); | ||
| appendChild(hostParent, comment, vcRefHost.view); |
There was a problem hiding this comment.
should we try to appendChild first, and only create the lContainerNode after? In case appendChild fails? (you should check the return value) or are we sure that it never happens?
There was a problem hiding this comment.
appendChild can be a noop if the parent can't insert native, but it only means that the native comment will then be appended later.
So it seems ok to me.
| } else if (action === WalkLNodeTreeAction.Destroy) { | ||
| ngDevMode && ngDevMode.rendererDestroyNode++; | ||
| (renderer as ProceduralRenderer3).destroyNode !(node.native !); | ||
| executeNodeAction(action, renderer !, parent, node.native !, beforeNode); |
There was a problem hiding this comment.
The comment on the function says:
NOTE: for performance reasons, the possible actions are inlined within the function instead of being passed as an argument.
Are you sure that you should use an external function? In which case you should probably remove that comment
There was a problem hiding this comment.
The note was moved to the right location.
|
Hi @marclaval! This PR has merge conflicts due to recent upstream merges. |
| @@ -137,33 +92,24 @@ function getNextOrParentSiblingNode(initialNode: LNode, rootNode: LNode): LNode| | |||
| if (node === rootNode) { | |||
| return null; | |||
| } | |||
| // When the walker exits a container, the beforeNode has to be restored to the previous value. | |||
| if (node && !node.pNextOrParent && node.tNode.type === TNodeType.Container) { | |||
| beforeNodeStack.pop(); | |||
There was a problem hiding this comment.
I am concerned that we have a stack which we have to push/pop. Could you explain more why do we need it? I would like to see if we could do without it?
There was a problem hiding this comment.
When we walk the LNode tree, we can enter and exit containers. In that case:
- on enter, the
beforeNodemust be changed to become the comment node of that container. - on exit, the
beforeNodemust be restored to the previous value, whatever it is.
Hence the stack.
The new unit test I've added in this PR is testing such scenario. It fails without the stack.
| @@ -185,36 +131,33 @@ function walkLNodeTree( | |||
| startingNode: LNode | null, rootNode: LNode, action: WalkLNodeTreeAction, renderer?: Renderer3, | |||
| renderParentNode?: LElementNode | null, beforeNode?: RNode | null) { | |||
| let node: LNode|null = startingNode; | |||
| let beforeNodeStack: (RNode | null | undefined)[] = [beforeNode]; | |||
There was a problem hiding this comment.
Originally the implementation did not need stack, now we do. How come?
There was a problem hiding this comment.
I don't think it was working. I think that the test I've added was failing.
| @@ -185,36 +131,33 @@ function walkLNodeTree( | |||
| startingNode: LNode | null, rootNode: LNode, action: WalkLNodeTreeAction, renderer?: Renderer3, | |||
There was a problem hiding this comment.
Can you fix the spelling of Optionnal and Optionnal above => Optional.
| .innerHTML.replace(/ style=""/g, '') | ||
| .replace(/ class=""/g, ''); | ||
| } | ||
| get html(): string { return toHtml(this.hostElement as any as Element); } |
There was a problem hiding this comment.
To only have one place where we clean the HTML output when testing
|
Rebased, comments addressed or answered, PTAL |
|
You can preview d9e7d84 at https://pr24346-d9e7d84.ngbuilds.io/. |
| } | ||
| } | ||
|
|
||
| expect(renderToHtml(Template, ctx)).toEqual('World'); |
There was a problem hiding this comment.
renderToHtml is deprecated, could you use TemplateFixture
| * World | ||
| * % } | ||
| * % } | ||
| */ |
There was a problem hiding this comment.
Can you help me understand why this example needs stack. My understanding is that the stack should not be needed.
When this test renders with condition2 == false the result is
<!--container condition2 -->World<!--container condition3--><!--container condition-->
NOTE: I added the condition for easier reasoning.
So once the condition2 == true the result should be
Hello<!--container condition2 -->World<!--container condition3--><!--container condition-->
There is no need to do any searches since the container attached at <!--container condition2 --> should just need to insert in front of the comment node. No searching or stack should be needed.
So I started to do some debugging and the behavior was a bit unexpected.
- When the outer most
container(0);executed the comment was inserted into the DOM immediately. (This is expected). - When the inner most
container(0);executed the code determined that it should not be inserted into DOM and delayed the insert. This is not expected. The delay in insertion means that the comments don't get inserted until outerembeddedViewEnd()is executed. And at this point it seems like you need the stack. But I think we should never get here if the comment would already be inserted. So I think this is where the issue is.
| */ | ||
| function Template(rf: RenderFlags, ctx: any) { | ||
| if (rf & RenderFlags.Create) { | ||
| { container(0); } |
There was a problem hiding this comment.
from comment above I am calling this the outer most container.
| { | ||
| if (rf1 & RenderFlags.Create) { | ||
| { container(0); } | ||
| { container(1); } |
There was a problem hiding this comment.
from comment above I am calling this the inner most container.
| containerRefreshEnd(); | ||
| } | ||
| } | ||
| embeddedViewEnd(); |
There was a problem hiding this comment.
from comment above I am calling this the outer embeddedViewEnd()
|
I've removed the stack by inlining the As discussed, we'll see how we can improve how embedded views are inserted in future refactors. |
|
You can preview 265b211 at https://pr24346-265b211.ngbuilds.io/. |
mhevery
added
action: merge
|
You can preview 9ed7da4 at https://pr24346-9ed7da4.ngbuilds.io/. |
|
You can preview 6eb0335 at https://pr24346-6eb0335.ngbuilds.io/. |
|
Currently I have a couple places where I put ng-container inside a component to apply a directive inside the host. (It uses intersection-observer API). Then I inject ElementRef to the directive, but I have to take nativeElement.parentElement in order to get the host element. (This is because it provides me with the comment node). No idea how common this is but it would be a breaking change if comment nodes went away - albeit probably take two seconds to fix. What would be the best way for a directive on ng-container to access its host element? |
|
Comment nodes are not going away, finally. |
|
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 brings back comment nodes for containers, in order to simplify the code and to ease the implementation of i18n.
Once the code base is more mature, we might consider removing them again.
The main issue with these nodes could be performance. So I've run the existing benchmarks with and without them (only a few runs on my local machine).
On the large table one which creates 202 containers, there is no measurable difference (i.e. too small to be visible in Benchpress).
On deep tree one which creates 8190 containers, there is definitely an impact at creation time only. The penalty is around 5ms (~10%) of script time.