fix(custom-element): batch custom element prop patching #13478
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
WalkthroughAdds begin/end patch lifecycle hooks for Vue custom elements, defers and batches prop updates during a patch, updates renderer to call the hooks around element patching, and adds tests validating batched prop updates and watcher behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant VueElement
participant ComponentInstance
Renderer->>VueElement: detect _isVueCE on n1.el
Renderer->>VueElement: _beginPatch() %% color: #DDEBF7 (begin)
Note right of VueElement: _patching = true\n_dirty = false
Renderer->>VueElement: set multiple props %% props set with shouldUpdate = false
VueElement->>VueElement: mark _dirty for changed props
Renderer->>Renderer: patchElement() %% actual VNode patch
Renderer->>VueElement: _endPatch() %% color: #E2F0D9 (end)
Note right of VueElement: _patching = false\nif _dirty -> trigger update on instance
VueElement->>ComponentInstance: trigger update (if _dirty)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changesNo out-of-scope functional code changes detected relative to the objectives in the linked issue. Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/runtime-dom/src/apiCustomElement.ts (2)
231-233: Consider clarifying lifecycle flags & guarding against stale_dirty
_patching+_dirtyare clear, but after an out-of-patch_update()call (e.g. prop changed via DOM API),_dirtystaystrue.
While harmless, a permanently-true flag is misleading when inspecting instances and invites future misuse.private _update() { const vnode = this._createVNode() if (this._app) vnode.appContext = this._app._context render(vnode, this._root) + // reset bookkeeping when we are outside a batched patch cycle + if (!this._patching) this._dirty = false }A one-liner reset keeps semantics tight.
494-502: Minor: avoid redundant_dirty = truewhen immediate update firesWhen
shouldUpdateistrueand_update()is called synchronously, there is no benefit in keeping_dirtyflagged.if (val !== this._props[key]) { - this._dirty = true + // mark dirty only when the render will be deferred + if (!shouldUpdate || this._patching) this._dirty = trueKeeps the flag strictly associated with deferred work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/runtime-core/src/component.ts(1 hunks)packages/runtime-core/src/renderer.ts(1 hunks)packages/runtime-dom/__tests__/customElement.spec.ts(1 hunks)packages/runtime-dom/src/apiCustomElement.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/src/renderer.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
VueElement(202-710)
🔇 Additional comments (4)
packages/runtime-core/src/component.ts (1)
1273-1280: Custom-element patch-lifecycle hooks exposed – looks goodThe addition of
_beginPatch()/_endPatch()in theComponentCustomElementInterfacealigns with the renderer changes and the concrete implementation inVueElement.
No issues spotted – signature and visibility (@internal) are appropriate.packages/runtime-dom/src/apiCustomElement.ts (2)
460-468:shouldUpdatecomputation may skip updates if instance not yet mountedInside the generated setter we pass
!this._patchingto_setProp.
During pre-mount cycles (e.g. attributes parsed beforeconnectedCallback),this._instanceis stillnull, so_setPropwill:
- mark
_dirty,- evaluate
shouldUpdate === true,- not call
_update()becausethis._instanceis falsy,- leave
_dirtyset totrue.If the element mounts without going through a renderer patch cycle (
_beginPatch/_endPatch), the first render happens with stale props.Please double-check that all early attribute/property writes are followed by either
_beginPatch/_endPatchor a later setter invocation so an update is eventually fired.
676-693: Great addition – batching hook integrates cleanly with rendererThe
_beginPatch / _endPatchpair is straightforward and thefinally{}invocation on the renderer side guarantees symmetry even on thrown errors – well done.packages/runtime-dom/__tests__/customElement.spec.ts (1)
477-565: Solid coverage for batched prop patchingThe two new test blocks exercise both sync and async scenarios and verify:
• DOM output
• watcher invocation counts
• no redundant updates on identical prop setsThis should reliably catch regressions in the new batching logic. 👍
Also applies to: 566-660
f695bc3 to
1354a62
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-core/src/renderer.ts (1)
624-627: Remove redundant!!and duplicate casts for clearer intent
customElementis derived solely to hold either the element reference ornull; the double-negation adds no value and forces an unnecessary boolean conversion.
Inlining the element reference also eliminates the repeatedas VueElementcast.- const customElement = !!(n1.el && (n1.el as VueElement)._isVueCE) - ? (n1.el as VueElement) - : null + const el = n1.el as VueElement | null + const customElement = el && el._isVueCE ? el : nullThis aligns with the Biome lint recommendation and slightly improves readability.
🧰 Tools
🪛 Biome (1.9.4)
[error] 624-624: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime-core/src/renderer.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/src/renderer.ts (1)
packages/runtime-dom/src/apiCustomElement.ts (1)
VueElement(202-710)
🪛 Biome (1.9.4)
packages/runtime-core/src/renderer.ts
[error] 624-624: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation
(lint/complexity/noExtraBooleanCast)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: test / unit-test-windows
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
close #12619
Applies prop updates on custom elements in batch before rendering instead of one at a time serially, resolving discrepancies in behavior compared to normal components.
Summary by CodeRabbit
New Features
Bug Fixes
Tests