refactor(ivy): insert embedded views immediately #24629
Conversation
marclaval
added
action: review
|
You can preview bca5208 at https://pr24629-bca5208.ngbuilds.io/. |
| * | ||
| * @param parent The parent in which to insert the child | ||
| * @param currentView The LView being processed | ||
| * @return boolean Whether the child element should be inserted. | ||
| */ | ||
| export function canInsertNativeNode(parent: LNode, currentView: LViewData): boolean { | ||
| const parentIsElement = parent.tNode.type === TNodeType.Element; | ||
| const parentIsView = parent.tNode.type === TNodeType.View; |
There was a problem hiding this comment.
I am not convinced that this logic is correct. My understanding is that we can insert view immediately if it is normal view, but always delay it if it is embedded view.
- normal view: A view as a result of a child component view or
% if() {block. (only exception is if it is projected node) - embedded view: A view created through
ViewContainerRef
Could we VC on this?
There was a problem hiding this comment.
but always delay it if it is embedded view
My understanding is there are 2 cases for embedded views:
- calling ViewContainerRef#createEmbeddedView - here we know the insertion point at the view creation time and could insert immediately
- calling TemplateRef#createEmbeddedView and then (eventually) inserting through ViewContainerRef#insert - here we need to delay (as a given view might be never inserted, at least in theory!)
So IMO there are cases where could insert embedded view at its creation time. Obviously it doesn't mean that we should if there is no perf gain and having 2 code paths makes the code more complex...
|
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 |
mhevery
added
action: merge
|
I see that you just added the 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. |
|
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 86464d6 at https://pr24629-86464d6.ngbuilds.io/. |
|
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 is a follow up of #24346 where @mhevery noted that the insertion of embedded views was currently always delayed, while in fact they can be inserted immediately in most cases.
The root change here is that the
canInsertNativeNode()method now returns true for views. It assumes that their parent container is already defined and inserted.The rest of the changes is some refactoring to adapt to the new logic.
Performance wise, it doesn't improve our current benchmarks, but it slightly increases the size of the default bundles (minified hello_world goes from 8kb to 8.2kb).