-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
fix(useTemplateRef): handle useTemplateRef edge case with vFor #12733
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-sfc
@vue/compiler-dom
@vue/compiler-ssr
@vue/runtime-core
@vue/runtime-dom
@vue/reactivity
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
I'm not sure whether it would be considered a problem, but this change would also impact |
Thanks for your review. Perhaps this cannot be regarded as a problem, because this kind of behavior already exists in PROD.
|
I would be nervous about drawing any conclusions based on that example. It strikes me as an unintentional edge case, rather than something we should be using as precedent. I'm not entirely clear why But, given |
WalkthroughA new test verifies that template refs collected via Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant Renderer
participant Reactivity
Component->>Renderer: Render elements with v-for and template ref
Renderer->>Renderer: Collect refs for each element
Renderer->>Reactivity: Wrap refs array with shallowReactive
Reactivity-->>Renderer: Return reactive array
Renderer->>Component: Assign reactive array to template ref
Assessment against linked issues
Possibly related issues
Poem
✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/runtime-core/src/rendererTemplateRef.ts (1)
126-134
: Inconsistent behaviour between string refs and object refs
shallowReactive([refValue])
is created only when_isString
is true.
For the_isRef
branch a plain array is still assigned (ref.value = [refValue]
), meaning that:<li :ref="myShallowRef" ref_for />will not receive a reactive array, whereas
<li ref="myKey" ref_for />will.
Unless there is a deliberate reason to preserve this asymmetry, consider applying the same
shallowReactive
wrapping to the_isRef
path for consistency and to avoid surprising API differences.- ref.value = [refValue] + ref.value = shallowReactive([refValue])
🧹 Nitpick comments (2)
packages/runtime-core/src/rendererTemplateRef.ts (1)
14-14
: Minor: keep import list alphabetised for easier diff-scanning
shallowReactive
was inserted betweenisRef
andtoRaw
, breaking the previous alphabetical order of the imported symbols. A consistent ordering reduces merge-conflicts and makes future diffs easier to read.packages/runtime-core/__tests__/helpers/useTemplateRef.spec.ts (1)
76-126
: Nice coverage – consider assertingisReactive(t1.value)
as wellThe new test correctly checks that
instance.refs['refKey']
is reactive, but it never verifies thatt1.value
– the value returned byuseTemplateRef
– is the same reactive proxy. A quick extra assertion would guarantee the public API receives the reactive array you expect:expect(t1!.value).toBe(currentInstance.refs['refKey']) expect(isReactive(t1!.value)).toBe(true)Adding this would catch a future regression where
refs
stays reactive butuseTemplateRef
returns a plain array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/runtime-core/__tests__/helpers/useTemplateRef.spec.ts
(2 hunks)packages/runtime-core/src/rendererTemplateRef.ts
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/runtime-core/__tests__/helpers/useTemplateRef.spec.ts (3)
packages/runtime-core/src/component.ts (1)
currentInstance
(708-708)packages/runtime-core/src/helpers/useTemplateRef.ts (1)
useTemplateRef
(8-40)packages/runtime-test/src/serialize.ts (1)
serializeInner
(22-33)
close #12731
the underlying problem is
Summary by CodeRabbit
Bug Fixes
Tests