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

lejunyang · 2024-11-16T10:01:51Z

~~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:~~
~~- extract unmount code, delete exposed properties when unmounting, clear parent and set resolved to false~~
~~- unmount if parent changed when connected~~
~~- delete invalid exposed key from exposed when mounting~~

changes:

extract observer code
extract unmount code, delete exposed properties when unmounting, clear parent
unmount if parent changed in connectedCallback and then re-mount
observe again and re-mount if it's resolved and no instance, new parent will be set on new mount
delete invalid exposed key from exposed when mounting

…ove element and append it back after fully unmounted (vuejs#12412)

github-actions · 2024-11-18T06:40:14Z

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 · 2024-11-18T06:40:23Z

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 · 2024-11-18T08:00:04Z

packages/runtime-dom/src/apiCustomElement.ts

-      } else if (__DEV__) {
-        warn(`Exposed property "${key}" already exists on custom element.`)
+      } else {
+        delete exposed[key] // delete it from exposed in case of deleting wrong exposed key when disconnected


Could you provide more info on why need this line?

This is to remove invalid exposed key.
Let's say we have an element with prop foo, and we accidentally expose an object {foo: 1}, it will be ignored as foo is already on this element. In disconnectedCallback, we'll delete exposed properties according to exposed, but deleting foo can cause an error as it's non-configurable prop.
Therefore, we should remove that key ahead

edison1105 · 2024-11-18T08:07:10Z

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 · 2024-11-18T08:39:05Z

@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 · 2024-11-18T09:02:59Z

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

edison1105 · 2024-11-18T14:10:34Z

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

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

…r element

…f again instead of only re-mount

lejunyang · 2024-11-19T03:42:02Z

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 · 2024-11-19T14:00:16Z

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 · 2024-11-22T02:22:06Z

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.

lejunyang · 2024-11-22T15:51:03Z

Updated, thanks for the info
Also I was correct, this._resolved = false will lead to applyStyle twice. unmount doesn't remove style nodes so we don't need to applyStyle again. I have added that in test case
So we just need to mount again instead of resolveDef again

fix(custom-element): fix parent, observer and exposed issues when rem…

0e913dc

…ove element and append it back after fully unmounted (vuejs#12412)

edison1105 reviewed Nov 18, 2024

View reviewed changes

fix(custom-element): should update when moving custom element to othe…

77da780

…r element

lejunyang marked this pull request as draft November 18, 2024 15:18

fix(custom-element): reset resolved and parent in unmount to resoveDe…

f81a841

…f again instead of only re-mount

lejunyang marked this pull request as ready for review November 18, 2024 15:40

edison1105 added the scope: custom elements label Nov 19, 2024

test: fix test case issue, outer element should be defined first

53077a5

edison1105 added the 🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. label Nov 22, 2024

edison1105 mentioned this pull request Nov 22, 2024

fix(customElement): preserve appContext during hmr reload #12455

Open

edison1105 changed the title ~~fix(custom-element): fix parent, observer and exposed issues when remove element and append it back immediately or after fully unmounted~~ fix(custom-element): properly handle custom element remove then insert again Nov 22, 2024

fix(custom-element): change solution

6b52371

lejunyang force-pushed the remove-append branch from 3c23328 to 6b52371 Compare November 27, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lejunyang commented Nov 16, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 18, 2024 •

edited

Loading

edison1105 Nov 18, 2024

lejunyang Nov 18, 2024 •

edited

Loading

edison1105 commented Nov 18, 2024

lejunyang commented Nov 18, 2024 •

edited

Loading

lejunyang commented Nov 18, 2024 •

edited

Loading

edison1105 commented Nov 18, 2024

lejunyang commented Nov 19, 2024

lejunyang commented Nov 19, 2024 •

edited

Loading

edison1105 commented Nov 22, 2024

lejunyang commented Nov 22, 2024

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

Are you sure you want to change the base?

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

Conversation

lejunyang commented Nov 16, 2024 • edited Loading

github-actions bot commented Nov 18, 2024 • edited Loading

Size Report

Bundles

Usages

pkg-pr-new bot commented Nov 18, 2024 • edited Loading

edison1105 Nov 18, 2024

Choose a reason for hiding this comment

lejunyang Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

edison1105 commented Nov 18, 2024

lejunyang commented Nov 18, 2024 • edited Loading

lejunyang commented Nov 18, 2024 • edited Loading

edison1105 commented Nov 18, 2024

lejunyang commented Nov 19, 2024

lejunyang commented Nov 19, 2024 • edited Loading

edison1105 commented Nov 22, 2024

lejunyang commented Nov 22, 2024

lejunyang commented Nov 16, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024 •

edited

Loading

pkg-pr-new bot commented Nov 18, 2024 •

edited

Loading

lejunyang Nov 18, 2024 •

edited

Loading

lejunyang commented Nov 18, 2024 •

edited

Loading

lejunyang commented Nov 18, 2024 •

edited

Loading

lejunyang commented Nov 19, 2024 •

edited

Loading