fix(reactivity): normalize toRef property keys before dep lookup + improve types#14625
fix(reactivity): normalize toRef property keys before dep lookup + improve types#14625edison1105 merged 5 commits intovuejs:mainfrom
Conversation
📝 WalkthroughWalkthroughWidened toRef/toRefs typings and runtime to accept PropertyKey keys (string | number | symbol), normalized internal ref keys (preserving symbols, coercing non-symbols), adjusted array index handling, and added tests verifying triggerRef behavior for numeric and Symbol keys on reactive sources. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/reactivity/src/ref.ts (1)
377-377: Minor: RedundantString()call for already-stringified keys.Since
this._keyis already a string for non-symbol keys (after line 370), theString()wrapper is only necessary for the symbol case. This is functionally correct but slightly redundant. The current approach is acceptable for clarity.Optional: Avoid redundant String() call
- if (!isArray(_object) || !isIntegerKey(String(this._key))) { + if (!isArray(_object) || isSymbol(this._key) || !isIntegerKey(this._key)) {This makes the intent clearer: symbols are never integer keys, and for strings we can check directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/reactivity/src/ref.ts` at line 377, The condition uses String(this._key) redundantly because this._key is already a string for non-symbol keys; replace the String(...) call with the raw key and, if you want to be explicit about symbol keys, guard with typeof: use this._key directly in isIntegerKey or use typeof this._key === 'symbol' ? false : isIntegerKey(this._key) so the check becomes if (!isArray(_object) || !isIntegerKey(this._key)) (or the equivalent guarded form) referencing isArray, isIntegerKey, _object and this._key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/reactivity/src/ref.ts`:
- Line 377: The condition uses String(this._key) redundantly because this._key
is already a string for non-symbol keys; replace the String(...) call with the
raw key and, if you want to be explicit about symbol keys, guard with typeof:
use this._key directly in isIntegerKey or use typeof this._key === 'symbol' ?
false : isIntegerKey(this._key) so the check becomes if (!isArray(_object) ||
!isIntegerKey(this._key)) (or the equivalent guarded form) referencing isArray,
isIntegerKey, _object and this._key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc76e1b6-41f1-413d-b327-2e5bcae01687
📒 Files selected for processing (2)
packages/reactivity/__tests__/ref.spec.tspackages/reactivity/src/ref.ts
|
/ecosystem-ci run |
|
📝 Ran ecosystem CI: Open
|
fix #12427
close #12431
Summary by CodeRabbit
New Features
Tests
Types