vuejs / core

🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
https://vuejs.org/
MIT License
47.75k stars 8.34k forks source link

fix(custom-element): properly handle custom element remove then insert again #12413

Open lejunyang opened 6 days ago

lejunyang commented 6 days ago

close https://github.com/vuejs/core/issues/12412

changes: - extract observer code - observe again and re-mount if it's resolved and no instance, new parent will be set on new mount - exposed properties should be configurable, and we should delete them if element is fully unmounted, so that we can expose again in new mount

changes:

github-actions[bot] commented 4 days ago

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB (+222 B) 38 kB (+79 B) 34.3 kB (+90 B)
vue.global.prod.js 158 kB (+222 B) 57.9 kB (+76 B) 51.5 kB (+82 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47.2 kB 18.3 kB 16.8 kB
createApp 55.2 kB 21.3 kB 19.5 kB
createSSRApp 59.3 kB 23.1 kB 21 kB
defineCustomElement 60.3 kB (+222 B) 23 kB (+67 B) 20.9 kB (+67 B)
overall 69.1 kB 26.5 kB 24.1 kB
pkg-pr-new[bot] commented 4 days ago

Open in Stackblitz

@vue/compiler-core

``` pnpm add https://pkg.pr.new/@vue/compiler-core@12413 ```

@vue/compiler-dom

``` pnpm add https://pkg.pr.new/@vue/compiler-dom@12413 ```

@vue/compiler-ssr

``` pnpm add https://pkg.pr.new/@vue/compiler-ssr@12413 ```

@vue/compiler-sfc

``` pnpm add https://pkg.pr.new/@vue/compiler-sfc@12413 ```

@vue/reactivity

``` pnpm add https://pkg.pr.new/@vue/reactivity@12413 ```

@vue/runtime-core

``` pnpm add https://pkg.pr.new/@vue/runtime-core@12413 ```

@vue/runtime-dom

``` pnpm add https://pkg.pr.new/@vue/runtime-dom@12413 ```

@vue/server-renderer

``` pnpm add https://pkg.pr.new/@vue/server-renderer@12413 ```

@vue/shared

``` pnpm add https://pkg.pr.new/@vue/shared@12413 ```

vue

``` pnpm add https://pkg.pr.new/vue@12413 ```

@vue/compat

``` pnpm add https://pkg.pr.new/@vue/compat@12413 ```

commit: 53077a5

edison1105 commented 4 days ago

Thank you for your contribution. I have some suggestions and questions.

Click "remove el2" and "append el2"

I think the reason for the error here is that _parent and _resolved were not reset when disconnected. Therefore, the correct fix should be:

// in disconnectedCallback
this._parent = undefined
this._resolved = false

and then click "change el2 attr", its attribute is changed, but prop is not updated

The fix to this part are generally correct. However, I don't understand why this line needs to be added.

extract observer code

I think this change is no longer necessary if reset _parent and _resolved in disconnectedCallback

lejunyang commented 4 days ago

@edison1105 Thanks for your suggestions! It's a good fix for current issues. However I found another issue regarding moving element. If we move a child element directly into other element(not waiting two nextTick), its parent context is not updated

I will check more on this and update issue and commit this night

lejunyang commented 4 days ago

Actually setting _resolved to false can cause some duplicate executions, like _applyStyles

edison1105 commented 4 days ago

Actually setting _resolved to false can cause some duplicate executions, like _applyStyles

re-mount should also re-apply the style, right ?

lejunyang commented 3 days ago

Oh, I forgot to run e2e tests in my local, my bad. I can see it's from _parseSlots, child custom element is removed and unmounted... let me see how to handle this

lejunyang commented 3 days ago

The reason work with Teleport (shadowRoot: false) test case fails is that: my-y is defined first, my-p is defined later, and it removes my-y and causes unmounting. I think this is not code issue, it's test case issue. We should always define outer element first, especially when SSR, otherwise inner element can't find correct parent, so I updated this test case

edison1105 commented 12 hours ago

While fixing #12453, I realized I made a mistake and my previous review was incorrect. Your initial submission might have been correct. this._resolved = false will lead to the following code never running. I apologize for this.

if (this._resolved) {
  this._setParent()
  this._update()
}

We should reuse the already resolved _def.