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.

Sign up for GitHub

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

Jump to bottom

fix(custom-element): fix parent, observer and exposed issues when remove element and append it back immediately or after fully unmounted #12413

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix(custom-element): fix parent, observer and exposed issues when remove element and append it back immediately or after fully unmounted #12413

wants to merge 4 commits into from
+80 −12

Conversation

lejunyang
Copy link

@lejunyang lejunyang commented Nov 16, 2024 •
edited
Loading

close #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:

  • 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

fix(custom-element): fix parent, observer and exposed issues when rem…
0e913dc 
…ove element and append it back after fully unmounted (vuejs#12412)
@github-actions GitHub Actions
Copy link

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

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 pkg.pr.new
Copy link

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

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
edison1105 reviewed Nov 18, 2024
View reviewed changes
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you provide more info on why need this line?

Copy link
Author

@lejunyang lejunyang Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Author

lejunyang commented Nov 18, 2024 •
edited
Loading

@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
Copy link
Author

lejunyang commented Nov 18, 2024 •
edited
Loading

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

@edison1105
Copy link
Member

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

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

@lejunyang lejunyang marked this pull request as draft November 18, 2024 15:18
@lejunyang lejunyang marked this pull request as ready for review November 18, 2024 15:40
@lejunyang lejunyang changed the title fix(custom-element): fix parent, observer and exposed issues when remove element and append it back after fully unmounted fix(custom-element): fix parent, observer and exposed issues when remove element and append it back immediately or after fully unmounted Nov 18, 2024
@lejunyang
Copy link
Author

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
Copy link
Author

lejunyang commented Nov 19, 2024 •
edited
Loading

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 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
Copy link
Member

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.

@edison1105 edison1105 mentioned this pull request Nov 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edison1105 edison1105 edison1105 left review comments

Assignees
No one assigned
Labels
🔨 p3-minor-bug Priority 3: this fixes a bug, but is an edge case that only affects very specific usage. scope: custom elements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove vue custom element and append it back later can cause a lot of issues
2 participants
@lejunyang @edison1105