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: |
WalkthroughThis change introduces patching lifecycle hooks ( Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer
participant CustomElement (el)
participant ComponentInstance
Renderer->>CustomElement: _beginPatch()
Renderer->>CustomElement: set multiple props
CustomElement->>CustomElement: mark _dirty = true for changed props
Renderer->>CustomElement: patchElement()
Renderer->>CustomElement: _endPatch()
CustomElement->>ComponentInstance: trigger update if _dirty and instance exists
Assessment against linked issues
Suggested reviewers
Poem
✨ 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 (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
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
| slotScopeIds, | ||
| optimized, | ||
| ) | ||
| if (n1.el && (n1.el as VueElement)._isVueCE) { |
There was a problem hiding this comment.
Can be changed to const isVueCE = !!(n1.el && (n1.el as any)._isVueCE) patchElement may cause n1 to be unmounted, but n1.el still has a value after patchElement
There was a problem hiding this comment.
Change applied
There was a problem hiding this comment.
Actually just tweaked slightly to something that I think is clearer and also has fewer casts, let me know what you think
edison1105
added
ready to merge
alex-snezhko
force-pushed
the
custom-element-batch-prop-patch
branch
from
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
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