Skip to content
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

Closed
wants to merge 19 commits into from

Conversation

baiwusanyu-c
Copy link
Member

@baiwusanyu-c baiwusanyu-c commented Oct 7, 2023

close: #9341
close: #8127

@github-actions
Copy link

github-actions bot commented Oct 7, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.5 kB (+342 B) 34.6 kB (+86 B) 31.2 kB (+71 B)
vue.global.prod.js 147 kB (+342 B) 54.1 kB (+81 B) 48.1 kB (+101 B)

Usages

Name Size Gzip Brotli
createApp 49.6 kB 19.5 kB 17.8 kB
createSSRApp 53.2 kB 20.9 kB 19.1 kB
defineCustomElement 52.2 kB (+342 B) 20.3 kB (+89 B) 18.5 kB (+85 B)
overall 63.1 kB 24.5 kB 22.3 kB

Co-authored-by: edison <daiwei521@126.com>
@baiwusanyu-c
Copy link
Member Author

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Oct 11, 2023

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools success success
nuxt success success
pinia success success
quasar failure failure
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros failure failure
vuetify success success
vueuse success success
vue-simple-compiler success success

@edison1105
Copy link
Member

also fix #8127

@yashha
Copy link

yashha commented Dec 14, 2023

@baiwusanyu-c
Found an issue with mounting custom elements within a vue subcomponent, that is rendered conditionally.
https://stackblitz.com/edit/vitejs-vite-eyrmms?file=package.json

@baiwusanyu-c
Copy link
Member Author

I will take the time to check the issue

Copy link

codspeed-hq bot commented Dec 15, 2023

CodSpeed Performance Report

Merging #9351 will improve performances by 73.58%

Comparing baiwusanyu-c:bwsy/fix/CENestedAsync (c02daeb) with main (04d2c05)

Summary

⚡ 1 improvements
✅ 52 untouched benchmarks

Benchmarks breakdown

Benchmark main baiwusanyu-c:bwsy/fix/CENestedAsync Change
write reactive obj, read 1000 computeds 68.6 ms 39.5 ms +73.58%

@baiwusanyu-c
Copy link
Member Author

@baiwusanyu-c Found an issue with mounting custom elements within a vue subcomponent, that is rendered conditionally. https://stackblitz.com/edit/vitejs-vite-eyrmms?file=package.json

Is your problem that <h1>Custom Component</h1> is not rendering after clicking the button?

@yashha
Copy link

yashha commented Dec 15, 2023

@baiwusanyu-c Yes, the shadowroot of the custom element is staying empty, without the patch it is working

@baiwusanyu-c
Copy link
Member Author

try again @yashha

@yashha
Copy link

yashha commented Dec 15, 2023

@baiwusanyu-c
In the minimal project in stack blitz it is working now properly
https://stackblitz.com/edit/vitejs-vite-v7r9rs?file=patches%2F%40vue%2Bruntime-dom%2B3.3.7.patch

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:

Uncaught (in promise) out of memory 4
uncaught exception: out of memory
Uncaught out of memory

@yashha
Copy link

yashha commented Dec 15, 2023

It was something on our side, we had a component without template tags only script tags, that caused the issue.

@baiwusanyu-c
Copy link
Member Author

It should be that references cause memory usage. I will consider how to solve this problem.

@SavkaTaras
Copy link

SavkaTaras commented Feb 5, 2024

Hello @baiwusanyu-c and @edison1105 ,
I hope you are doing well.

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,
Taras Savka

@SavkaTaras
Copy link

Hello @baiwusanyu-c @edison1105 @yyx990803 @LinusBorg ,
I hope you guys are doing well.

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,
Taras Savka

@@ -1,4 +1,5 @@
import {
type Ref,
Copy link
Contributor

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.

Comment on lines +729 to +730
})
test('async & multiple levels of nested custom elements', async () => {
Copy link
Contributor

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())
Copy link
Contributor

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 to null 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.

yyx990803 added a commit that referenced this pull request Aug 6, 2024
@yyx990803
Copy link
Member

Superseded by 37ccb9b which is based on this PR and reuses tests from this PR. Thanks!

@yyx990803 yyx990803 closed this Aug 6, 2024
@baiwusanyu-c baiwusanyu-c deleted the bwsy/fix/CENestedAsync branch August 8, 2024 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Rejected
7 participants