Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: async subscription do not run when mutate state synchronously after patch #2870

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions packages/pinia/__tests__/subscriptions.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,5 +353,35 @@ describe('Subscriptions', () => {
expect(spy1).toHaveBeenCalledTimes(1)
expect(spy2).toHaveBeenCalledTimes(2)
})

it('debugger events should not be array when subscription is not trigger by patch', () => {
const store = useStore()
store.$subscribe(
({ type, events }) => {
if (type === MutationType.direct) {
expect(Array.isArray(events)).toBe(false)
}
},
{ flush: 'sync' }
)
store.$patch({ user: 'Edu' })
store.user = 'a'
})

it('should trigger subscription when mutate state synchronously after patch', async () => {
const store = useStore()
const spy1 = vi.fn()
const spy2 = vi.fn()
const spy3 = vi.fn()
store.$subscribe(spy1, { flush: 'sync' })
store.$subscribe(spy2, { flush: 'pre' })
store.$subscribe(spy3, { flush: 'post' })
store.$patch({ user: 'Edu' })
store.user = 'a'
expect(spy1).toHaveBeenCalledTimes(2)
await nextTick()
expect(spy2).toHaveBeenCalledTimes(2)
expect(spy3).toHaveBeenCalledTimes(2)
})
})
})
50 changes: 25 additions & 25 deletions packages/pinia/src/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
ref,
set,
del,
nextTick,
isVue2,
} from 'vue-demi'
import {
Expand Down Expand Up @@ -256,7 +255,7 @@ function createSetupStore<
if (isListening) {
debuggerEvents = event
// avoid triggering this while the store is being built and the state is being set in pinia
} else if (isListening == false && !store._hotUpdating) {
} else if (isListening === false && !store._hotUpdating) {
// let patch send all the events together later
/* istanbul ignore else */
if (Array.isArray(debuggerEvents)) {
Expand All @@ -271,8 +270,8 @@ function createSetupStore<
}

// internal state
let isListening: boolean // set to true at the end
let isSyncListening: boolean // set to true at the end
let isListening = false // set to true at the end
let shouldTrigger = false // The initial value does not matter, and no need to set to true at the end
let subscriptions: SubscriptionCallback<S>[] = []
let actionSubscriptions: StoreOnActionListener<Id, S, G, A>[] = []
let debuggerEvents: DebuggerEvent[] | DebuggerEvent
Expand All @@ -291,9 +290,6 @@ function createSetupStore<

const hotState = ref({} as S)

// avoid triggering too many listeners
// https://github.com/vuejs/pinia/issues/1129
let activeListener: Symbol | undefined
function $patch(stateMutation: (state: UnwrapRef<S>) => void): void
function $patch(partialState: _DeepPartial<UnwrapRef<S>>): void
function $patch(
Expand All @@ -302,7 +298,7 @@ function createSetupStore<
| ((state: UnwrapRef<S>) => void)
): void {
let subscriptionMutation: SubscriptionCallbackMutation<S>
isListening = isSyncListening = false
isListening = false
// reset the debugger events since patches are sync
/* istanbul ignore else */
if (__DEV__) {
Expand All @@ -324,13 +320,7 @@ function createSetupStore<
events: debuggerEvents as DebuggerEvent[],
}
}
const myListenerId = (activeListener = Symbol())
nextTick().then(() => {
if (activeListener === myListenerId) {
isListening = true
}
})
isSyncListening = true
isListening = true
// because we paused the watcher, we need to manually call the subscriptions
triggerSubscriptions(
subscriptions,
Expand Down Expand Up @@ -454,11 +444,19 @@ function createSetupStore<
options.detached,
() => stopWatcher()
)
const stopWatcher = scope.run(() =>
watch(
const stopWatcher = scope.run(() => {
const stop1 = watch(
() => pinia.state.value[$id],
() => {
shouldTrigger = isListening
},
{ deep: true, flush: 'sync' }
)

const stop2 = watch(
() => pinia.state.value[$id] as UnwrapRef<S>,
(state) => {
if (options.flush === 'sync' ? isSyncListening : isListening) {
if (shouldTrigger) {
callback(
{
storeId: $id,
Expand All @@ -471,7 +469,14 @@ function createSetupStore<
},
assign({}, $subscribeOptions, options)
)
)!

const stop = () => {
stop1()
stop2()
}

return stop
})!

return removeSubscription
},
Expand Down Expand Up @@ -647,12 +652,8 @@ function createSetupStore<

// avoid devtools logging this as a mutation
isListening = false
isSyncListening = false
pinia.state.value[$id] = toRef(newStore._hmrPayload, 'hotState')
isSyncListening = true
nextTick().then(() => {
isListening = true
})
isListening = true

for (const actionName in newStore._hmrPayload.actions) {
const actionFn: _Method = newStore[actionName]
Expand Down Expand Up @@ -779,7 +780,6 @@ function createSetupStore<
}

isListening = true
isSyncListening = true
return store
}

Expand Down