fix: async subscription do not run when mutate state synchronously after patch #2870
Conversation
✅ Deploy Preview for pinia-official canceled.
|
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Thanks! I will take a look when I can |
yangmingshan
force-pushed
the
subscribe
branch
from
60b9c6d to
7844dba
Compare
|
This PR is now based on Does |
|
Thanks! I think it's fine to keep this for v3 only so don't worry about it |
commit: |
| () => { | ||
| shouldTrigger = isListening | ||
| }, | ||
| { deep: true, flush: 'sync' } |
There was a problem hiding this comment.
I'm revisitting this PR and realizing that this might impact perf too much
There was a problem hiding this comment.
I need to see in the playground the impact
There was a problem hiding this comment.
Without checking the playground, I think we can't go this way: #610
WalkthroughThe PR adds two subscription tests and refactors Pinia's store subscription logic: replaces multi-flag listening/locking with a single shouldTrigger flag, removes nextTick-based listening transitions, and splits a single watcher into two coordinated watchers for patch and direct-change handling. Changes
Sequence Diagram(s)sequenceDiagram
rect lightblue
participant User
participant Store
participant WatcherFull as Watcher (full state)
participant WatcherDirect as Watcher (direct/patch)
participant Subscribers
end
User->>Store: perform patch / HMR payload
Store->>Store: set isListening = false\nset shouldTrigger = true
Store->>WatcherFull: emit (full-state watcher)
WatcherFull->>Store: check shouldTrigger
alt shouldTrigger true
WatcherFull->>Subscribers: notify with patched events
end
Store->>Store: set isListening = true
User->>Store: perform direct mutation
Store->>Store: set isListening = false\nset shouldTrigger = true
Store->>WatcherDirect: emit (direct-change watcher)
WatcherDirect->>Store: check shouldTrigger
alt shouldTrigger true
WatcherDirect->>Subscribers: notify with direct-change events
end
Store->>Store: set isListening = true
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 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: 0
🧹 Nitpick comments (2)
packages/pinia/__tests__/subscriptions.spec.ts (1)
357-357: Fix grammatical error in test description.The test name contains a grammatical error: "not be an array" should be "not an array".
Apply this diff:
- it('debuggerEvents is not be an array when subscription is not trigger by patch', () => { + it('debuggerEvents is not an array when subscription is not triggered by patch', () => {packages/pinia/src/store.ts (1)
267-268: Consider clarifying the shouldTrigger initialization.While the comment states "The initial value does not matter", initializing
shouldTriggertoundefinedmay cause confusion during debugging. Consider either:
- Initializing to
falsefor clarity, or- Expanding the comment to explain that it's set synchronously before being checked by async watchers
Apply this diff:
- let shouldTrigger: boolean | undefined // The initial value does not matter, and no need to set to true at the end + // Captures the listening state synchronously for async watchers; set by sync watcher before async watchers check it + let shouldTrigger: boolean | undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/pinia/__tests__/subscriptions.spec.ts(1 hunks)packages/pinia/src/store.ts(7 hunks)
🔇 Additional comments (5)
packages/pinia/__tests__/subscriptions.spec.ts (1)
371-385: Good test coverage for the fix.This test effectively validates the core fix for issue #992, ensuring that subscriptions with all flush strategies ('sync', 'pre', 'post') are properly triggered when state is synchronously mutated after a patch operation.
packages/pinia/src/store.ts (4)
252-252: Condition refinement looks good.The added
!store._hotUpdatingcheck appropriately prevents debugger event accumulation during hot module replacement operations.
291-291: isListening state management is correct.The bookending of the patch operation with
isListening = false/trueproperly prevents automatic watcher firing during the patch, while manual triggering (lines 315-319) ensures subscribers are notified once.Also applies to: 313-313
624-624: HMR state transition is consistent.The synchronous
isListening = trueassignment aligns with the removal of nextTick-based state transitions and maintains consistency with the$patchimplementation.
437-467: Dual-watcher design appears intentional; performance concern is valid but unproven.Analysis reveals this is a deliberate implementation to handle timing correctly: during state mutations,
shouldTriggercaptures the currentisListeningstate synchronously viastop1, thenstop2fires the callback only if that captured state permits it. The design prevents subscriptions from firing at incorrect times due to race conditions with theisListeningflag.The performance overhead concern is valid—two watchers per subscription with deep watching on every mutation—but lacks empirical measurement. The codebase contains no performance benchmarks or complaints about subscription overhead. The trade-off appears accepted for correctness.
Before accepting or modifying this approach, measure actual performance impact in your usage patterns. If performance is acceptable, no changes needed. If degradation is significant, consider profiling to identify if this dual-watcher is the bottleneck or if other optimizations would suffice.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3 #2870 +/- ##
==========================================
- Coverage 91.01% 91.00% -0.01%
==========================================
Files 17 17
Lines 1402 1401 -1
Branches 211 209 -2
==========================================
- Hits 1276 1275 -1
Misses 125 125
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Fix #992
I run into same issue when I porting Pinia to Vue Mini, and I found another debugger issue for sync subscription (please see the test).
I managed a way to fix them, the fix may have extra memory / performance costs, I didn't do any measurements. But I used it in Pinia for Vue Mini anyway.
Summary by CodeRabbit
Tests
Improvements