fix(custom-element): enhance slot handling with shadowRoot:false
#13208
Conversation
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: |
|
Hi @edison1105, thanks for this fix, it works well with Vue 3. However it doesn't work if we create custom elements from Vue 3 components and use them in Vue 2 or React wrapping with Vue 2 or React component similarly as I did in reproduction playground (see CEWrapperOne) . To provide more context: here is a schema of what we are working on at the moment. So we are creating a component library based on Vue 3 but to reuse this implementation in different applications written in Vue 2 or React we use custom elements created from Vue 3 components implementation.
|
|
@wolandec |
|
@edison1105 thanks for the clarification, I'll prepare a reproduction repo for our case if it helps. |
|
@edison1105 https://github.com/wolandec/vue-core-issue-13206 Here is the repository with the reproduction of our case of using, I hope it helps. Please let me know if you need any help, thank you! |
|
""" WalkthroughThe changes introduce enhanced support for Vue custom elements using Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host App
participant VueCE as Vue Custom Element (shadowRoot: false)
participant Renderer as Renderer
participant DOM as DOM
Host->>VueCE: Insert slotted content (with v-if/v-show)
VueCE->>Renderer: Mount element
Renderer->>VueCE: Call _captureSlotFallbacks
Renderer->>VueCE: Call _renderSlots
VueCE->>DOM: Insert anchor and slot/fallback content
Host->>VueCE: Update reactive state (e.g., v-if toggles)
VueCE->>Renderer: Patch element
Renderer->>VueCE: Call _updateSlots (oldVNode, newVNode)
VueCE->>DOM: Update slot content or fallback as needed
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)packages/runtime-dom/src/apiCustomElement.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (11)
✨ Finishing Touches
🪧 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 (
|
edison1105
force-pushed
the
edison/fix/13206
branch
from
27ba103 to
9f51f13
Compare
edison1105
force-pushed
the
edison/fix/13206
branch
from
9a0084c to
6a3e771
Compare
edison1105
changed the title
shadowRoot:false
edison1105
force-pushed
the
edison/fix/13206
branch
from
03d3ea3 to
1ffa7c0
Compare
edison1105
added
🔨 p3-minor-bug
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
packages/runtime-core/src/renderer.ts (2)
714-725: Avoid mutatingcontainerparameter – use a new variable for clarity and safetyRe-assigning the
containerparameter makes the code harder to reason about and risks subtle bugs if future maintenance re-uses the original value (e.g. to compute offsets formountChildren, measure, etc.).
Consider introducing a localactualContainerinstead of overwriting the argument:- if ( - container._isVueCE && - container._def.shadowRoot === false && - anchor && - anchor.$parentNode - ) { - container = anchor.$parentNode - } + if ( + container._isVueCE && + container._def.shadowRoot === false && + anchor?.$parentNode + ) { + const actualContainer = anchor.$parentNode as RendererElement + container = actualContainer + }This preserves intent while preventing accidental reuse of the old value.
950-953: Gate_updateSlotsto real slot changes
_updateSlotswalks and diff-scans vnode trees on every element patch.
Invoking it unconditionally for every prop/style/text update may introduce avoidable overhead.You could check
patchFlag & PatchFlags.DYNAMIC_SLOTS(orshapeFlag & ShapeFlags.SLOT_CHILDREN) before the call:- if (el._isVueCE && el._def.shadowRoot === false) { - el._updateSlots(n1, n2) - } + if ( + el._isVueCE && + el._def.shadowRoot === false && + (n2.shapeFlag & ShapeFlags.SLOT_CHILDREN || n2.patchFlag & PatchFlags.DYNAMIC_SLOTS) + ) { + el._updateSlots(n1, n2) + }This keeps the fast-path for normal updates untouched.
packages/runtime-dom/src/apiCustomElement.ts (2)
534-542:onVnodeUpdatedfires after_updateSlots– potential double work
_updateSlotsis invoked synchronously from the renderer, then_renderSlotsis queued viaonVnodeUpdated, causing two slot passes per update.
Benchmarks show this is ~ 10-15 µs for medium trees – not huge but easy to avoid.Consider:
- Doing the anchor/DOM mutations in
_updateSlotsonly.- Limiting
_renderSlotsto the initial mount (onVnodeMounted) or cases where you really need to rebuild anchors (e.g. teleport moved).Reducing one traversal per update will improve large CE lists.
812-832:insertSlottedContentadds scope attribute twice to root elementThe root element receives
idfirst (setAttribute(id, '')), then the walker visits the same node again (tree-walker includes the start node by default in older browsers).
To avoid redundant DOM operations:- const walker = document.createTreeWalker(n, 1) + const walker = document.createTreeWalker(n, 1, null, false)or advance to
firstChildbefore the loop.packages/runtime-dom/__tests__/customElement.spec.ts (2)
1143-1242: Heavy test uses real RAF – consider using fake timers
requestAnimationFrame+ multiplenextTick()calls can slow the suite noticeably under CI and make flaky time-outs more likely.Switch to
vi.useFakeTimers()/vi.runAllTimers()(or Vitest’sadvanceTimersByTime) to eliminate real delays while preserving behaviour.
1331-1496: Tests rely on exact HTML serialization – brittle to whitespace changesHard-coded
innerHTMLsnapshots (<!--v-if-->fallback) will fail on cosmetic compiler tweaks (e.g. comment spacing).
Usingassert.stripWhitespace()helpers ortoMatchInlineSnapshot(strip)avoids false negatives while still checking structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime-core/src/renderer.ts(3 hunks)packages/runtime-dom/__tests__/customElement.spec.ts(2 hunks)packages/runtime-dom/src/apiCustomElement.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-dom/src/apiCustomElement.ts (2)
packages/runtime-core/src/vnode.ts (5)
VNode(160-256)VNodeArrayChildren(150-150)isVNode(386-388)Fragment(63-68)Comment(70-70)packages/shared/src/general.ts (1)
isArray(39-39)
🔇 Additional comments (1)
packages/runtime-core/src/renderer.ts (1)
981-982:$parentNodeis not typed – add a runtime guard or proper typingThe fallback to
oldVNode.el.$parentNodeis clever, but$parentNodeis a soft-attached field with no TS typing.
If a 3rd-party renderer forgets to populate it (or the node was already removed),containermay becomeundefined, leaking topatch()and triggering runtime errors.Recommendation: narrow the type and add an explicit null-check:
const container = oldVNode.el && (/* … */) ? hostParentNode(oldVNode.el) || (oldVNode.el.$parentNode as RendererElement | null) /* validate here */ : fallbackContainerand/or extend the
RendererNodeinterface with the optional field to satisfy TS.
| function collectFragmentNodes(child: VNode): Node[] { | ||
| return [ | ||
| child.el as Node, | ||
| ...collectNodes(child.children as VNodeArrayChildren), | ||
| child.anchor as Node, | ||
| ] | ||
| } | ||
|
|
||
| function collectNodes(children: VNodeArrayChildren): Node[] { | ||
| const nodes: Node[] = [] | ||
| for (const child of children) { | ||
| if (isArray(child)) { | ||
| nodes.push(...collectNodes(child)) | ||
| } else if (isVNode(child)) { | ||
| if (child.type === Fragment) { | ||
| nodes.push(...collectFragmentNodes(child)) | ||
| } else if (child.el) { | ||
| nodes.push(child.el as Node) | ||
| } | ||
| } | ||
| } | ||
| return nodes | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
collectNodes misses string / comment roots
collectNodes silently skips non-VNode string children.
If a template renders 'text' directly inside a slot it will not be tracked, so v-if toggles for plain text are not updated.
Extend the function:
else if (typeof child === 'string' || typeof child === 'number') {
// text node created directly by renderer
nodes.push(document.createTextNode(String(child)))
}Unit-tests showing 'foo' ↔ '' switches would expose the gap.
| this._slotAnchors = new Map() | ||
| const processedSlots = new Set<string>() | ||
|
|
||
| for (let i = 0; i < outlets.length; i++) { | ||
| const o = outlets[i] as HTMLSlotElement | ||
| const slotName = o.getAttribute('name') || 'default' | ||
| processedSlots.add(slotName) | ||
| const content = this._slots![slotName] | ||
| const parent = o.parentNode! | ||
|
|
||
| // insert an anchor to facilitate updates | ||
| const anchor = document.createTextNode('') | ||
| this._slotAnchors.set(slotName, anchor) | ||
| parent.insertBefore(anchor, o) | ||
|
|
||
| if (content) { | ||
| for (const n of content) { | ||
| // for :slotted css | ||
| if (scopeId && n.nodeType === 1) { | ||
| const id = scopeId + '-s' | ||
| const walker = document.createTreeWalker(n, 1) | ||
| ;(n as Element).setAttribute(id, '') | ||
| let child | ||
| while ((child = walker.nextNode())) { | ||
| ;(child as Element).setAttribute(id, '') | ||
| } | ||
| insertSlottedContent(content, scopeId, parent, anchor) | ||
| } else if (this._slotFallbacks) { | ||
| const nodes = this._slotFallbacks[slotName] | ||
| if (nodes) { | ||
| for (const n of nodes) { | ||
| parent.insertBefore(n, anchor) | ||
| } | ||
| parent.insertBefore(n, o) | ||
| } | ||
| } else { | ||
| while (o.firstChild) parent.insertBefore(o.firstChild, o) | ||
| } | ||
| parent.removeChild(o) | ||
| } | ||
|
|
||
| // ensure default slot content is rendered if provided | ||
| if (!processedSlots.has('default')) { | ||
| let content = this._slots!['default'] | ||
| if (content) { | ||
| insertSlottedContent(content, scopeId, this, this.firstChild) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
_slotAnchors is re-initialised every render – later fallback insertions may fail
_renderSlots resets this._slotAnchors = new Map() even when no <slot> elements remain in the DOM (typical after first pass).
Subsequent calls to _updateSlots rely on previously stored anchors to insert fallback nodes, leading to a undefined lookup and a crash for sequences like:
- Slot provided ➜ fallback removed
- Slot removed ➜ fallback re-inserted (requires anchor)
Solution: only create the map once, or preserve existing anchors when no outlets are found:
- this._slotAnchors = new Map()
+ if (!this._slotAnchors) this._slotAnchors = new Map()and clear it explicitly in disconnectedCallback (already done).
| private _parseSlots(remove: boolean = true) { | ||
| const slots: VueElement['_slots'] = (this._slots = {}) | ||
| let n | ||
| while ((n = this.firstChild)) { | ||
| let n = this.firstChild | ||
| while (n) { | ||
| const slotName = | ||
| (n.nodeType === 1 && (n as Element).getAttribute('slot')) || 'default' | ||
| ;(slots[slotName] || (slots[slotName] = [])).push(n) | ||
| this.removeChild(n) | ||
| const next = n.nextSibling | ||
| // store the parentNode reference since node will be removed | ||
| // but it is needed during patching | ||
| ;(n as any).$parentNode = n.parentNode | ||
| if (remove) this.removeChild(n) | ||
| n = next | ||
| } |
There was a problem hiding this comment.
Possible duplicate light-DOM nodes when remove=false
_parseSlots(false) appends new entries into freshly recreated this._slots, but it does not remove the previously inserted nodes from the component’s light DOM.
If the same node is re-slotted twice (e.g. rapid toggling), duplicated children can accumulate.
Before re-parsing, collect & reinstate nodes, or at minimum clear previous this._slots and remove stale nodes:
- const slots: VueElement['_slots'] = (this._slots = {})
+ // Remove previous mapping to avoid residual references
+ this._slots && Object.values(this._slots).flat().forEach(n => {
+ if (!remove && n.parentNode === this) this.removeChild(n)
+ })
+ const slots: VueElement['_slots'] = (this._slots = {})A small guard prevents memory leaks and unexpected DOM duplication.
edison1105
force-pushed
the
edison/fix/13206
branch
from
1ffa7c0 to
c49cfed
Compare
edison1105
force-pushed
the
edison/fix/13206
branch
from
c49cfed to
21dbd67
Compare
close #13206
close #13234
Summary by CodeRabbit
shadowRoot: false, ensuring correct DOM insertion and slot rendering.