fix(custom-element): enhance slot handling with shadowRoot:false
#13208
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/runtime-core
@vue/reactivity
@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 _parseSlotFallbacks and _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. ✨ 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 (
|
27ba103 to
9f51f13
Compare
9a0084c to
6a3e771
Compare
shadowRoot:false
03d3ea3 to
1ffa7c0
Compare
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
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.
c49cfed to
21dbd67
Compare
21dbd67 to
2d7e5af
Compare
db19df8 to
404ff99
Compare
8b240da to
3534188
Compare
_renderSlots only renders slots that have corresponding outlets, but not all slots in this._slots are rendered to the DOM tree. During a full diff, parent.insertBefore(node,anchor) will throw an error because the anchor is not in the DOM tree.
3534188 to
3e72409
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/runtime-dom/src/apiCustomElement.ts (3)
628-641:⚠️ Potential issuePotential memory leaks and duplicate DOM nodes with
remove=false.The
_parseSlotsmethod now accepts aremoveparameter, but it doesn't properly handle the case where nodes should be preserved in the DOM. When called withremove=false, it appends new entries tothis._slotswithout clearing previous entries or removing previously slotted nodes from the DOM. This can lead to duplicate nodes and memory leaks.Apply this diff to properly clean up previous slot mappings:
private _parseSlots(remove: boolean = true) { const slots: VueElement['_slots'] = (this._slots = {}) + // Remove previous mapping to avoid residual references + if (this._slots && !remove) { + Object.values(this._slots).flat().forEach(n => { + if (n.parentNode === this) this.removeChild(n) + }) + } let n = this.firstChild while (n) {
814-842:⚠️ Potential issueNode collection helpers miss text nodes.
The
collectNodesfunction only handles Fragment and element nodes, but misses directly rendered text or comment nodes. If a slot contains plain text (e.g.,{{ message }}or'text'), those nodes won't be tracked properly, causing issues with conditional rendering.Extend the function to handle text nodes:
function collectNodes( children: VNodeArrayChildren, ): (Node & { $parentNode?: 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) } + } else if (typeof child === 'string' || typeof child === 'number') { + // text node created directly by renderer + nodes.push(document.createTextNode(String(child))) } } return nodes }
719-728:⚠️ Potential issueAnchor lookup can crash when
_slotAnchorsentry is missing.Line 724 unconditionally dereferences
this._slotAnchors!.get(name)!without checking if the anchor exists. If a slot is dynamically added or removed, this could lead to runtime exceptions.Add a defensive check:
- const anchor = this._slotAnchors!.get(name)! + const anchor = this._slotAnchors?.get(name) + if (!anchor) continue // Skip if anchor not found
🧹 Nitpick comments (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
247-249: New private properties for slot management without shadow DOM.The additional properties provide the necessary infrastructure to handle slots when
shadowRoot: false:
_slotswith$parentNodereference tracks nodes and their original parents_slotFallbacksstores default content from slots_slotAnchorsmaintains DOM anchor points for slot renderingConsider adding JSDoc comments to explain the purpose of each property for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/runtime-core/src/renderer.ts(2 hunks)packages/runtime-dom/__tests__/customElement.spec.ts(2 hunks)packages/runtime-dom/src/apiCustomElement.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/runtime-core/src/renderer.ts
- packages/runtime-dom/tests/customElement.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-dom/src/apiCustomElement.ts (4)
packages/runtime-dom/src/index.ts (1)
VueElement(259-259)packages/runtime-core/src/vnode.ts (5)
VNode(160-256)VNodeArrayChildren(150-150)isVNode(386-388)Fragment(63-68)Comment(70-70)packages/runtime-core/src/hydration.ts (1)
isComment(83-84)packages/shared/src/general.ts (1)
isArray(39-39)
🔇 Additional comments (8)
packages/runtime-dom/src/apiCustomElement.ts (8)
22-22: Appropriate imports added for enhanced slot handling.The imports of
Fragment,VNodeArrayChildren, andisVNodesupport the new slot handling functionality for custom elements without shadow DOM.Also applies to: 28-28, 33-33
340-342: Proper cleanup of slot-related properties.The
disconnectedCallbackmethod now properly cleans up all slot-related properties, preventing memory leaks.
537-541: Lifecycle hooks for custom element slot management.The changes appropriately hook into the vnode lifecycle to:
- Parse fallback slot content on mount
- Render slots initially
- Re-render slots when the component updates
This ensures proper slot handling when not using shadow DOM.
657-664: Good anchor creation for DOM patching.Adding text node anchors in the DOM is a solid approach for maintaining slot positions. Using a Map to store these anchors for later reference is efficient and appropriate.
677-679: Proper parent node tracking for DOM patching.Storing the parent node reference on slotted nodes and inserting them before the anchor ensures correct DOM placement during updates.
680-686: Good fallback content handling.The code now properly renders fallback content when no slot content is provided, which is essential for custom elements without shadow DOM.
695-710: Handling v-if node switching in slots.This section handles v-if conditional rendering by replacing nodes in the
_slotscollection when they change, ensuring that future renders use the correct nodes.
744-758: Effective fallback content extraction.The
_parseSlotFallbacksmethod efficiently extracts and stores fallback content from slot elements for later use.
close #13206
close #13234
Note
This PR only fixed bugs in the optimized mode, while issues still exist in the full diff scenario. This happens because
_renderSlotsonly renders slots that have corresponding outlets, meaning not all slots inthis._slotsare rendered to the DOM tree. During full diff,parent.insertBefore(node, anchor)throws an error because the anchor node isn't actually in the DOM tree.I couldn't find a suitable solution, so rolled back the full diff related code in 3e72409
Summary by CodeRabbit
Summary by CodeRabbit
shadowRoot: false, ensuring correct DOM insertion and slot rendering.