fix: trigger reactivity for shallowRef when using object syntax #3041
Conversation
✅ Deploy Preview for pinia-official canceled.
|
WalkthroughDetects and triggers Vue shallowRef reactivity when store.$patch merges plain objects into shallow refs; adds tests covering shallowRef, markRaw, nested/partial patches, and multiple shallowRefs to validate behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component / Watcher
participant S as Store (reactive)
participant P as store.$patch
participant M as mergeReactiveObjects
participant Raw as toRaw(store)
participant SR as shallowRef
participant V as Vue Reactivity
C->>S: depend on shallowRef state
S->>P: call $patch(partialState)
P->>M: merge partial into reactive store state
M-->>P: merge complete
P->>Raw: inspect toRaw(store) for shallowRefs
alt patched shallowRef found
P->>SR: call triggerRef(rawTarget[key])
SR->>V: notify dependents
V-->>C: watchers/computeds rerun
else no shallowRef affected
P-->>S: patch completes without manual trigger
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/pinia/src/store.ts (2)
96-102: Avoid direct.hasOwnProperty; use the safe prototype call.
targetcan be a proxy or an object without a prototype. PreferObject.prototype.hasOwnProperty.call(target, key)to avoid edge cases. The extra guard here is also redundant becauseisPlainObject(targetValue)already implies “own” in this context.Apply this diff:
- target.hasOwnProperty(key) && + Object.prototype.hasOwnProperty.call(target, key) &&
151-158:isShallowRefrelies on a private flag; add a type and a defensive check.Using
__v_isShallowis internal API. It’s fine pragmatically, but please (a) narrow the return type toShallowRef<any>and (b) guard with strict equality to avoid truthy pitfalls.You can enhance it like this (type-only import outside this hunk):
// import type { ShallowRef } from 'vue' function isShallowRef(value: any): value is ShallowRef<any> { return isRef(value) && (value as any).__v_isShallow === true }packages/pinia/__tests__/store.patch.spec.ts (1)
219-401: Great coverage for shallowRef + object‑syntax$patch.Happy path, nested objects, function syntax baseline, direct assignment baseline, partial updates, and multi‑ref patches are all exercised.
Consider adding:
- A negative test to ensure watchers don’t fire on unrelated keys in the same
$patch.- A case where the shallowRef is patched with a non‑plain object (e.g., Date or Array) to assert intended behavior (trigger or not), documenting expectations.
Example to append:
it('does not trigger when patching unrelated key', async () => { const store = useShallowRefStore() const spy = vi.fn() const stop = watchEffect(() => { void store.counter.count; spy() }) spy.mockClear() store.$patch({ nestedCounter: { simple: 123 } }) await nextTick() expect(spy).not.toHaveBeenCalled() stop() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/pinia/__tests__/store.patch.spec.ts(2 hunks)packages/pinia/src/store.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/pinia/__tests__/store.patch.spec.ts (1)
packages/pinia/src/store.ts (1)
defineStore(878-973)
packages/pinia/src/store.ts (1)
packages/pinia/src/types.ts (1)
isPlainObject(19-29)
🔇 Additional comments (2)
packages/pinia/src/store.ts (1)
23-24: Import triggerRef: LGTM.Needed to manually notify dependents of shallow refs after object‑syntax patches.
packages/pinia/__tests__/store.patch.spec.ts (1)
1-2: Test imports: LGTM.Covers the added shallowRef scenarios with
computed,watchEffect, andnextTick.
|
@coderabbitai generate docstrings |
✅ Actions performedDocstrings generation started. |
Docstrings generation was requested by @doubledare704. * #3041 (comment) The following files were modified: * `packages/pinia/src/store.ts`
|
Note Generated docstrings for this pull request at #3043 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pinia/src/store.ts (1)
96-103: Use safehasOwnPropertyto avoid prototype edge cases.Direct calls can throw for objects with null prototypes and are weaker against prototype pollution scenarios.
- target.hasOwnProperty(key) && + Object.prototype.hasOwnProperty.call(target, key) &&Optional consistency tweak for the guard above (outside the changed hunk but same function):
- if (!patchToApply.hasOwnProperty(key)) continue + if (!Object.prototype.hasOwnProperty.call(patchToApply, key)) continue
🧹 Nitpick comments (1)
packages/pinia/src/store.ts (1)
528-528: Nit: remove leftover “no‑op” comment.It’s self‑evident from the code and can be dropped to reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pinia/src/store.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pinia/src/store.ts (1)
packages/pinia/src/types.ts (1)
isPlainObject(19-29)
🔇 Additional comments (1)
packages/pinia/src/store.ts (1)
323-339: ShallowRef reactivity trigger after object‑syntax patch: LGTM.Good use of
toRaw(store)and own‑key filtering; triggering once per patched shallowRef is correct.
- Add triggerRef import from Vue - Implement isShallowRef utility function to detect shallowRef using __v_isShallow - Modify method to detect shallowRef targets and trigger reactivity after patching - Add comprehensive tests for shallowRef reactivity scenarios - Ensure compatibility with existing patch behavior close vuejs#2861
- Replace setupStoreRef with toRaw(store) to eliminate coupling and state drift - Use Object.prototype.hasOwnProperty.call() for safer property checking - Improve type safety with Ref[] instead of any[] - Remove unnecessary setupStoreRef assignment and variable declaration - Maintain same functionality with more robust implementation
- Replace internal __v_isShallow with public isShallow API for Vue version compatibility - Fix hasOwnProperty usage in mergeReactiveObjects to prevent prototype pollution - Remove unnecessary no-op comment for cleaner code - Maintain all existing functionality while improving code quality
posva
force-pushed
the
fix/shallow-ref-patch-reactivity
branch
from
a66b124 to
780fab8
Compare
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3 #3041 +/- ##
==========================================
+ Coverage 90.90% 90.93% +0.02%
==========================================
Files 19 19
Lines 1683 1687 +4
Branches 237 238 +1
==========================================
+ Hits 1530 1534 +4
Misses 151 151
Partials 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/pinia/__tests__/store.patch.spec.ts (2)
247-290: Remove unnecessaryasynckeywords.Tests 1-3 are declared as
asyncfunctions but don't useawaitanywhere. Since these tests execute synchronously (usingflush: 'sync'in watchers), theasynckeyword is unnecessary and potentially misleading.Apply this diff to remove the unnecessary async keywords:
- it('does not trigger reactivity when patching marked raw', async () => { + it('does not trigger reactivity when patching marked raw', () => { const store = useShallowRefStore() // ... }) - it('triggers reactivity when patching shallowRef with object syntax', async () => { + it('triggers reactivity when patching shallowRef with object syntax', () => { const store = useShallowRefStore() // ... }) - it('triggers reactivity when patching nested properties in shallowRef', async () => { + it('triggers reactivity when patching nested properties in shallowRef', () => { const store = useShallowRefStore() // ... })
292-357: Remove unnecessaryasynckeywords.Tests 4-7 (baseline and edge case tests) are declared as
asyncfunctions but don't useawaitanywhere. Consistent with the earlier tests, these should not be async since they execute synchronously.Apply this diff to remove the unnecessary async keywords:
- it('works with function syntax (baseline test)', async () => { + it('works with function syntax (baseline test)', () => { const store = useShallowRefStore() // ... }) - it('works with direct assignment (baseline test)', async () => { + it('works with direct assignment (baseline test)', () => { const store = useShallowRefStore() // ... }) - it('handles partial updates correctly', async () => { + it('handles partial updates correctly', () => { const store = useShallowRefStore() // ... }) - it('works with multiple shallowRefs in single patch', async () => { + it('works with multiple shallowRefs in single patch', () => { const store = useShallowRefStore() // ... })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/pinia/__tests__/store.patch.spec.ts(2 hunks)packages/pinia/src/store.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pinia/src/store.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pinia/__tests__/store.patch.spec.ts (1)
packages/pinia/__tests__/vitest-mock-warn.ts (1)
mockWarn(123-125)
🔇 Additional comments (3)
packages/pinia/__tests__/store.patch.spec.ts (3)
1-7: LGTM!The new imports and mockWarn setup are appropriate for the shallowRef reactivity tests. The mockWarn() call at the describe block level will apply to all tests in the suite.
223-245: LGTM!The test store setup is well-designed with multiple shallowRef instances to support independent test scenarios and edge cases. The inclusion of
markedRawfor testing non-reactive behavior is particularly thoughtful.
222-358: Excellent test coverage for the shallowRef reactivity fix!The new test suite comprehensively validates the fix for issue #2861. It includes:
✅ Core functionality (object syntax with shallowRef)
✅ Edge cases (markRaw non-reactivity, nested properties, partial updates)
✅ Baseline tests (function syntax and direct assignment still work)
✅ Batch operations (multiple shallowRefs in one patch)The use of
flush: 'sync'ensures synchronous watcher execution for predictable testing, and the spy pattern withmockClear()properly isolates each test from the initial watcher trigger.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pinia/src/store.ts(6 hunks)
🔇 Additional comments (3)
packages/pinia/src/store.ts (3)
11-56: LGTM! Import changes support the shallowRef fix.The additions of
isShallowandtriggerRefenable the core fix, and converting runtime imports to type-only imports where appropriate is a good practice that reduces bundle size.
96-97: Good approach to access raw refs.Using
toRaw(target)allows inspection of the actual ref objects without Vue's automatic unwrapping, which is essential for detecting and triggering shallow refs.
120-124: Core fix correctly triggers shallow ref reactivity.This logic properly addresses the issue where object-syntax
$patchdoesn't trigger reactivity for shallow refs:
rawTarget[key]accesses the actual ref object (not unwrapped)isShallow()detects shallow refs using Vue's public APItriggerRef()manually notifies Vue's reactivity systemThe placement after assignment (line 118) is correct because the reactive proxy updates the ref's value, but the ref object itself remains the same, allowing us to trigger it.
| var subPatch = patchToApply[key] | ||
| var targetValue = target[key] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use const instead of var for loop variables.
These variables are not reassigned within the loop body, so const is more appropriate and prevents accidental reassignment. Using var is a code smell in modern JavaScript and provides function scope rather than block scope.
Apply this diff:
- var subPatch = patchToApply[key]
- var targetValue = target[key]
+ const subPatch = patchToApply[key]
+ const targetValue = target[key]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var subPatch = patchToApply[key] | |
| var targetValue = target[key] | |
| const subPatch = patchToApply[key] | |
| const targetValue = target[key] |
🤖 Prompt for AI Agents
In packages/pinia/src/store.ts around lines 102 to 103, the loop variables are
declared with var (var subPatch = patchToApply[key]; var targetValue =
target[key]) but they are never reassigned; replace var with const for both
declarations to use block-scoped, read-only bindings and avoid accidental
reassignment. Update the two declarations to use const and run tests/lint to
ensure no other references rely on var semantics.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/pinia/__tests__/store.patch.spec.ts (1)
222-354: Consider removing unnecessaryasynckeywords and adding edge case coverage.The test suite is comprehensive and well-structured. A few optional improvements:
All test functions are declared
asyncbut contain noawaitoperations. While harmless, removingasyncwould make the intent clearer.Consider adding edge case tests for completeness:
- Patching a shallowRef with
nullorundefined- Patching with primitive values
- Patching when the patch value itself is a ref or shallowRef
These are nice-to-have improvements; the current coverage validates the core functionality effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/pinia/__tests__/store.patch.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/pinia/__tests__/store.patch.spec.ts (1)
packages/pinia/__tests__/vitest-mock-warn.ts (1)
mockWarn(123-125)
🔇 Additional comments (8)
packages/pinia/__tests__/store.patch.spec.ts (8)
1-7: LGTM! Imports and setup are appropriate.The new imports (
vi,shallowRef,markRaw,watch) andmockWarnsetup align well with the test requirements for verifying shallowRef reactivity behavior with watchers.
222-241: LGTM! Store setup provides comprehensive test coverage.The
useShallowRefStorefunction creates a well-structured test store with:
- Simple shallowRef (
counter)- markRaw for negative testing (
markedRaw)- Nested shallowRef structure (
nestedCounter)This setup effectively supports all the test scenarios in the suite.
243-255: LGTM! markRaw control test is correct.This negative test correctly verifies that
markRawobjects don't trigger reactivity when patched, ensuring the fix doesn't inadvertently affectmarkRawbehavior.
257-267: LGTM! Core shallowRef reactivity test is sound.This test directly validates the fix for issue #2861, ensuring that
$patchwith object syntax correctly triggers reactivity for shallowRef state.
269-286: LGTM! Nested property test validates deep reactivity.Correctly verifies that patching nested properties within a shallowRef triggers watchers on deeply nested paths.
288-312: LGTM! Baseline tests ensure no regressions.These tests verify that the fix doesn't break existing functionality:
- Function syntax patching (already worked)
- Direct assignment (already worked)
Good defensive testing to catch potential regressions.
314-333: LGTM! Partial update test validates merge behavior.This test correctly verifies that
$patchmerges partial objects into shallowRef values rather than replacing them entirely, which aligns with the overall$patchsemantics. The explicit comment on Line 327 helps clarify the expected behavior.
335-353: LGTM! Multi-shallowRef test ensures batch handling.Correctly verifies that a single
$patchcall triggers reactivity for all affected shallowRefs, ensuring the implementation handles multiple shallowRefs in one batch.
2 participants
close #2861
Summary by CodeRabbit
Bug Fixes
Tests