fix(custom-element): properly handle custom element remove then insert again #12413
Conversation
…ove element and append it back after fully unmounted (vuejs#12412)
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-ssr
@vue/compiler-sfc
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
| } 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 |
There was a problem hiding this comment.
Could you provide more info on why need this line?
There was a problem hiding this comment.
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
|
Thank you for your contribution. I have some suggestions and questions.
I think the reason for the error here is that // in disconnectedCallback
this._parent = undefined
this._resolved = false
The fix to this part are generally correct. However, I don't understand why this line needs to be added.
I think this change is no longer necessary if reset |
|
@edison1105 Thanks for your suggestions! It's a good fix for current issues. I will check more on this and update issue and commit this night |
|
Actually setting |
re-mount should also re-apply the style, right ? |
…f again instead of only re-mount
lejunyang
changed the title
|
Oh, I forgot to run e2e tests in my local, my bad. I can see it's from |
|
The reason |
edison1105
added
the
🔨 p3-minor-bug
|
While fixing #12453, I realized I made a mistake and my previous review was incorrect. Your initial submission might have been correct. if (this._resolved) {
this._setParent()
this._update()
}We should reuse the already resolved |
edison1105
changed the title
|
Updated, thanks for the info |
lejunyang
force-pushed
the
remove-append
branch
from
3c23328 to
6b52371
Compare
|
Hey team any updates on this one? |
|
I just tested this Branch: pnpm add https://pkg.pr.new/vue@12413The fix solves this issue. Thanks @lejunyang |
WalkthroughThis update refactors the custom element lifecycle in Vue's runtime-dom package, modularizing unmounting and mutation observer logic. It adds comprehensive tests for removing, re-appending, and moving custom elements, verifying correct context, property, and style handling. The changes address issues with attribute updates, exposed properties, and parent-child relationships after DOM manipulations. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CustomElement (VueElement)
participant VueAppInstance
Browser->>CustomElement: connectedCallback()
CustomElement->>CustomElement: Check parent change
alt Parent changed and previously mounted
CustomElement->>CustomElement: _unmount(keepParentAndOb = true)
end
CustomElement->>CustomElement: _mount()
CustomElement->>VueAppInstance: Initialize app
Browser->>CustomElement: disconnectedCallback()
CustomElement->>CustomElement: _unmount()
CustomElement->>VueAppInstance: Unmount app
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/runtime-dom/__tests__/customElement.spec.ts (2)
167-176: Minor typing nit – return-type annotation
text: () => stringtechnically warns in strict TS becausestringhere is a type, not a value.
It still compiles due to contextual typing, but writingtext: () => string | undefined(or omitting the annotation altogether) avoids the redundant type literal while keeping full type-safety.- text: () => props.msg, + text: () => props.msg,The implementation is fine; this is just a cosmetic typing tip.
190-203: Good move test – one follow-up ideaThe test currently covers parent context changes.
Consider also asserting that the old parent’s context is no longer injected into the moved child (i.e. it doesn’t update when the old parent’smsgchanges). This guards against stale provide/inject references.packages/runtime-dom/src/apiCustomElement.ts (3)
289-304: Parent-change re-mount logic is correct but can double-observeAfter
_unmount(true)the formerMutationObserveris still active.
Because_instanceis nownull, the subsequentif (!this._instance) this._observe()re-invokesobserve()on an already observing instance.
While the spec silently replaces the previous options, it still costs an extra “disconnect+observe” cycle and can trigger redundant mutation records in certain polyfills.- // no instance means no observer - if (!this._instance) this._observe() + // restart observer only if it was disconnected by _unmount + if (!this._instance && !this._ob) this._observe()
358-368: Observer helper extracted – small optimisationNice extraction!
Two quick wins:
- You can pass
attributeFilterto avoid receiving style/class changes that Vue never reflects.- In
_setPropyou reconnect the observer for every reflected write. You can avoid thedisconnect/observeround-trip whenshouldReflectisfalse.Not critical, but it avoids needless micro-tasks and reduces the mutation backlog in attribute-heavy components.
Also applies to: 544-545
456-465: Configurable expose getters – 👍 with a cautionMaking the descriptors configurable fixes the deletion error.
However, the else-branch silently drops conflicting exposed keys. That can hide user mistakes at run-time.Consider throwing (or at least warning in prod) so integrators discover the conflict early:
- delete exposed[key] - if (__DEV__) - warn(`Exposed property "${key}" already exists on custom element.`) + delete exposed[key] + warn( + `Exposed property "${key}" conflicts with an existing property and ` + + `was ignored. Rename the exposed property to avoid collisions.`, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime-dom/__tests__/customElement.spec.ts(1 hunks)packages/runtime-dom/src/apiCustomElement.ts(5 hunks)packages/vue/__tests__/e2e/ssr-custom-element.spec.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/runtime-dom/src/apiCustomElement.ts
[error] 336-336: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
packages/vue/__tests__/e2e/ssr-custom-element.spec.ts (1)
112-112: Order-switch looks goodRe-registering the parent (
my-p) before the child (my-y) removes the race condition that caused the earlier hydration error – nice catch.packages/runtime-dom/__tests__/customElement.spec.ts (1)
150-188: Great coverage – verifies style & context after a full disconnectThese assertions catch three common regressions:
- exposed methods are cleared,
- style sheets aren’t duplicated, and
- re-insertion updates reactive context.
Nice work adding the double
nextTick()to ensure the element is fully disconnected before re-mounting.
| private _unmount(keepParentAndOb?: boolean) { | ||
| if (!keepParentAndOb) { | ||
| this._parent = undefined | ||
| if (this._ob) { | ||
| this._ob.disconnect() | ||
| this._ob = null | ||
| } | ||
| } | ||
| this._app && this._app.unmount() | ||
| if (this._instance) { | ||
| const exposed = this._instance.exposed | ||
| if (exposed) { | ||
| for (const key in exposed) { | ||
| delete this[key as keyof this] | ||
| } | ||
| } | ||
| this._instance.ce = undefined | ||
| } | ||
| this._app = this._instance = null | ||
| } |
There was a problem hiding this comment.
_unmount(): ensure internal state cleared before deleting exposed keys
delete this[key] is executed before _instance is nulled.
If any exposed getter references this._instance, the getter may run during deletion and access a stale instance.
Moving the this._instance = null assignment above the exposed-key loop eliminates this edge case:
- if (this._instance) {
+ const inst = this._instance
+ this._instance = null // prevent stale access in getters
+ if (inst) {
- const exposed = this._instance.exposed
+ const exposed = inst.exposedCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 336-336: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
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 mountchanges:- 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 mountingchanges:
Summary by CodeRabbit