-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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(custom-element): Use asynchronous custom element nesting to avoid errors #9351
Conversation
Size ReportBundles
Usages
|
Co-authored-by: edison <daiwei521@126.com>
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
also fix #8127 |
@baiwusanyu-c |
I will take the time to check the issue |
CodSpeed Performance ReportMerging #9351 will improve performances by 73.58%Comparing Summary
Benchmarks breakdown
|
Is your problem that |
@baiwusanyu-c Yes, the shadowroot of the custom element is staying empty, without the patch it is working |
try again @yashha |
@baiwusanyu-c But in our project we have now bad performance in dev mode, we run in some kind of loop, I don't know whats the cause. I get several of these:
|
It was something on our side, we had a component without template tags only script tags, that caused the issue. |
It should be that references cause memory usage. I will consider how to solve this problem. |
# Conflicts: # packages/runtime-dom/__tests__/customElement.spec.ts
Hello @baiwusanyu-c and @edison1105 , Could you please review this and merge if possible? We ran into this issue as well, when wc imports wc within the slot. Thank you, |
Hello @baiwusanyu-c @edison1105 @yyx990803 @LinusBorg , I am so sorry for bothering you all, but could you please take a look to see if this could be merged, and if so, please merge it? We are waiting on this PR. Thank you again for all your great work, |
@@ -1,4 +1,5 @@ | |||
import { | |||
type Ref, |
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.
This seems to be duplicated.
}) | ||
test('async & multiple levels of nested custom elements', async () => { |
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.
We normally have a blank line between tests.
// The asynchronous custom element needs to call | ||
// the resolveDef function of the descendant custom element at the end. | ||
if (this._ce_children) { | ||
this._ce_children.forEach(child => child._resolveDef()) |
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.
I'm wondering about what happens if children are removed from the DOM:
- Should we set
this._ce_children
back tonull
once we're done, so references to the children aren't retained indefinitely? - Do we need to check that the children haven't been removed while the parent was resolving?
- Perhaps children should remove themselves from this array when they are removed from the DOM?
Here's an example. The async component has a child, which is removed before the async component resolves. But because the component is in this._ce_children
it still gets mounted:
The logging shows that the child is still mounted, which doesn't seem right.
I think it's also worth noting that there's a similar problem when the async component itself is removed. This problem already exists on main
:
The problem in this second example is probably outside the scope of this PR, though maybe both of these examples could be fixed in the same way? I'm not sure.
Superseded by 37ccb9b which is based on this PR and reuses tests from this PR. Thanks! |
close: #9341
close: #8127