fix: trigger reactivity for shallowRef when using object syntax #3041
Conversation
- 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
✅ Deploy Preview for pinia-official canceled.
|
WalkthroughAdds tests covering shallowRef reactivity with store.$patch and updates setup-store $patch logic to detect patched shallowRefs (via toRaw) and call triggerRef for shallowRef values that received plain-object merges so Vue reactivity is notified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Component / Watcher
participant S as Setup Store
participant P as store.$patch
participant M as mergeReactiveObjects
participant Raw as toRaw(store)
participant R as shallowRef(s)
participant V as Vue Reactivity
C->>S: depend on shallowRef-based state (computed/watch)
S->>P: call $patch(partialState)
P->>M: merge partial into reactive store state
M-->>P: merge complete
P->>Raw: inspect toRaw(store) for shallowRefs affected
alt patched shallowRef found (plain-object update)
P->>R: call triggerRef() on each affected shallowRef
R->>V: notify dependents
V-->>C: computed/watch rerun
else no shallowRef affected
P-->>S: patch completes (no manual trigger)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
- 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
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.
- 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
close #2861
Summary by CodeRabbit
Bug Fixes
Tests