Fix HMR re-run plugins on existing store after HMR to refresh stale closures - #3169
Fix HMR re-run plugins on existing store after HMR to refresh stale closures#3169KavehKarami wants to merge 1 commit into
Conversation
✅ Deploy Preview for pinia-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughPinia now preserves subscriptions during HMR and re-runs plugins on existing stores using refreshed effect scopes. HMR tests cover ChangesHMR subscriptions and plugins
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HMR
participant PiniaStore
participant Plugins
participant Subscriptions
HMR->>PiniaStore: Apply _hotUpdate
PiniaStore->>Plugins: Re-run plugins in fresh scope
Plugins-->>PiniaStore: Return refreshed extensions and callbacks
PiniaStore->>Subscriptions: Preserve existing subscriptions
Subscriptions-->>PiniaStore: Trigger updated callbacks
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/pinia/__tests__/hmr.spec.ts (1)
583-639: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert call counts, not just the last captured value.
Both tests only check the final
capturedValue. IfrunPlugins()leaves the old plugin subscription registered alongside the new one, both callbacks fire and the last-registered one wins, so these assertions still pass while subscriptions silently accumulate on every HMR cycle. Adding avi.fn()per plugin run (or asserting the number of registered handlers) would actually pin the intended behavior.🧪 Suggested strengthening
let closureValue = 'original' let capturedValue = '' + const handler = vi.fn() pinia.use(({ store }) => { const current = closureValue // captured at plugin-run time store.$subscribe(() => { + handler(current) capturedValue = current }) }) @@ store.$patch({ n: 2 }) // _hotUpdate re-runs plugins on the existing store with the new closure → capturedValue becomes 'updated' expect(capturedValue).toBe('updated') + // fails if the stale plugin subscription is still registered + expect(handler).toHaveBeenCalledTimes(2) + expect(handler).toHaveBeenLastCalledWith('updated')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/pinia/__tests__/hmr.spec.ts` around lines 583 - 639, Strengthen both HMR plugin tests around the `pinia.use` callbacks and their `$subscribe`/`$onAction` registrations by tracking invocation counts with a fresh `vi.fn()` for each plugin run. After the initial action, assert only the original handler ran; after HMR and the second action, assert the new handler ran once and the old handler did not run again, proving subscriptions are replaced rather than accumulated while retaining the closure-value assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/pinia/__tests__/hmr.spec.ts`:
- Around line 583-639: Strengthen both HMR plugin tests around the `pinia.use`
callbacks and their `$subscribe`/`$onAction` registrations by tracking
invocation counts with a fresh `vi.fn()` for each plugin run. After the initial
action, assert only the original handler ran; after HMR and the second action,
assert the new handler ran once and the old handler did not run again, proving
subscriptions are replaced rather than accumulated while retaining the
closure-value assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28eacffb-6456-4906-b4a7-f2c4abb40c69
📒 Files selected for processing (2)
packages/pinia/__tests__/hmr.spec.tspackages/pinia/src/store.ts
close #991
Fix: plugin subscriptions retain stale closures after HMR (#991)
Problem
When a pinia.use() plugin registers a $subscribe or $onAction callback, the callback closes over values from the store module at the time the plugin runs. After HMR fires, the module reloads with updated code, but Pinia was running plugins on the temporary hot store (__hot:) rather than on the existing store. This meant the existing store's plugin subscriptions were never refreshed — they kept stale closures from before the HMR cycle, silently ignoring any logic changes in the new module.
Root cause
In createSetupStore, the plugin application loop ran unconditionally — including when hot = true (i.e., when building the ephemeral hot store used as a diff target). After _hotUpdate applied the diff to the existing store, the hot store was discarded but the existing store's plugin subscriptions were never re-run with the fresh closures from the updated module.
Solution
Tests
Two new regression tests in describe('both') cover the fix:
Both tests fail on the unfixed code and pass with the fix. Two additional sanity tests verify that user-level $subscribe and $onAction subscriptions (not added via plugins) survive HMR unchanged.
Summary by CodeRabbit