From fb5ddbad0502fe65e8d0e5950637e8fab4cd3c58 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 1 Sep 2023 09:29:52 +0100 Subject: [PATCH 1/3] Revert "Merge pull request #1 from reedsy/reactivity-fix" This reverts commit 704d43ee6d5263cb54aeae5bf8abee3f686d9a64, reversing changes made to 7b024d8bc7ed8e273b8bd853b1e23ec78ab1b045. --- src/store-util.js | 25 ------------------------- test/unit/modules.spec.js | 29 +---------------------------- 2 files changed, 1 insertion(+), 53 deletions(-) diff --git a/src/store-util.js b/src/store-util.js index c138a49f9..e3ba18609 100644 --- a/src/store-util.js +++ b/src/store-util.js @@ -30,8 +30,6 @@ export function resetStore (store, hot) { export function resetStoreState (store, state, hot) { const oldState = store._state const oldScope = store._scope - const oldCache = store._computedCache - const oldGettersKeySet = new Set(store.getters ? Object.keys(store.getters) : []) // bind store public getters store.getters = {} @@ -47,10 +45,6 @@ export function resetStoreState (store, state, hot) { scope.run(() => { forEachValue(wrappedGetters, (fn, key) => { - // Filter stale getters' key by comparing oldGetters and wrappedGetters, - // the key does not be removed from oldGettersKeySet are the key of stale computed cache. - // Stale computed cache: the computed cache should be removed as the corresponding module is removed. - oldGettersKeySet.delete(key) // use computed to leverage its lazy-caching mechanism // direct inline function use will lead to closure preserving oldState. // using partial to return function with only arguments preserved in closure environment. @@ -70,7 +64,6 @@ export function resetStoreState (store, state, hot) { // register the newly created effect scope to the store so that we can // dispose the effects when this method runs again in the future. store._scope = scope - store._computedCache = computedCache // enable strict mode for new state if (store.strict) { @@ -89,24 +82,6 @@ export function resetStoreState (store, state, hot) { // dispose previously registered effect scope if there is one. if (oldScope) { - const deadEffects = [] - const staleComputedCache = new Set() - oldGettersKeySet.forEach((staleKey) => { - staleComputedCache.add(oldCache[staleKey]) - }) - oldScope.effects.forEach(effect => { - // Use the staleComputedCache match the computed property of reactiveEffect, - // to specify the stale cache - if (effect.deps.length && !staleComputedCache.has(effect.computed)) { - // Merge the effect that already have dependencies and prevent from being killed. - scope.effects.push(effect) - } else { - // Collect the dead effects. - deadEffects.push(effect) - } - }) - // Dispose the dead effects. - oldScope.effects = deadEffects oldScope.stop() } } diff --git a/test/unit/modules.spec.js b/test/unit/modules.spec.js index 7eae663dc..9c6e96e46 100644 --- a/test/unit/modules.spec.js +++ b/test/unit/modules.spec.js @@ -1,4 +1,4 @@ -import { computed, h, nextTick } from 'vue' +import { h, nextTick } from 'vue' import { mount } from 'test/helpers' import Vuex from '@/index' @@ -925,31 +925,4 @@ describe('Modules', () => { /getters should be function but "getters\.test" in module "foo\.bar" is true/ ) }) - - it('module: computed getter should be reactive after module registration', () => { - const store = new Vuex.Store({ - state: { - foo: 0 - }, - getters: { - getFoo: state => state.foo - }, - mutations: { - incrementFoo: state => state.foo++ - } - }) - - const computedFoo = computed(() => store.getters.getFoo) - store.commit('incrementFoo') - expect(computedFoo.value).toBe(1) - - store.registerModule('bar', { - state: { - bar: 0 - } - }) - - store.commit('incrementFoo') - expect(computedFoo.value).toBe(2) - }) }) From b5d790ae70accfd53a8c8c901386ad44e2404450 Mon Sep 17 00:00:00 2001 From: "pei.fan" Date: Fri, 9 Dec 2022 16:49:05 +0900 Subject: [PATCH 2/3] test: add test case --- test/unit/modules.spec.js | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/test/unit/modules.spec.js b/test/unit/modules.spec.js index 9c6e96e46..7eae663dc 100644 --- a/test/unit/modules.spec.js +++ b/test/unit/modules.spec.js @@ -1,4 +1,4 @@ -import { h, nextTick } from 'vue' +import { computed, h, nextTick } from 'vue' import { mount } from 'test/helpers' import Vuex from '@/index' @@ -925,4 +925,31 @@ describe('Modules', () => { /getters should be function but "getters\.test" in module "foo\.bar" is true/ ) }) + + it('module: computed getter should be reactive after module registration', () => { + const store = new Vuex.Store({ + state: { + foo: 0 + }, + getters: { + getFoo: state => state.foo + }, + mutations: { + incrementFoo: state => state.foo++ + } + }) + + const computedFoo = computed(() => store.getters.getFoo) + store.commit('incrementFoo') + expect(computedFoo.value).toBe(1) + + store.registerModule('bar', { + state: { + bar: 0 + } + }) + + store.commit('incrementFoo') + expect(computedFoo.value).toBe(2) + }) }) From 7058043ff65c6a98388a4bb8d0c8faf6cf9cd156 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Thu, 31 Aug 2023 15:33:16 +0100 Subject: [PATCH 3/3] fix: avoid resetting store state when registering a dynamic module Fixes https://github.com/vuejs/vuex/issues/2197 At the moment, when registering a dynamic module, we call `resetStoreState()` just to register the getters for the new module. It seems unnecessary to reset the entire store state in this case, and this actually also leads to [other issues][1]. This change is based on the test case added in https://github.com/vuejs/vuex/pull/2201 The approach taken in this change is to refactor the getter registration into its own function, and call that new method when registering a dynamic module instead of resetting the store state. [1]: https://github.com/vuejs/vuex/issues/2197 --- src/store-util.js | 47 +++++++++++++++++++++++++---------------------- src/store.js | 9 ++++++--- 2 files changed, 31 insertions(+), 25 deletions(-) diff --git a/src/store-util.js b/src/store-util.js index e3ba18609..5cbe9aa2f 100644 --- a/src/store-util.js +++ b/src/store-util.js @@ -1,5 +1,5 @@ import { reactive, computed, watch, effectScope } from 'vue' -import { forEachValue, isObject, isPromise, assert, partial } from './util' +import { isObject, isPromise, assert, partial } from './util' export function genericSubscribe (fn, subs, options) { if (subs.indexOf(fn) < 0) { @@ -35,36 +35,19 @@ export function resetStoreState (store, state, hot) { store.getters = {} // reset local getters cache store._makeLocalGettersCache = Object.create(null) - const wrappedGetters = store._wrappedGetters - const computedObj = {} - const computedCache = {} // create a new effect scope and create computed object inside it to avoid // getters (computed) getting destroyed on component unmount. const scope = effectScope(true) - - scope.run(() => { - forEachValue(wrappedGetters, (fn, key) => { - // use computed to leverage its lazy-caching mechanism - // direct inline function use will lead to closure preserving oldState. - // using partial to return function with only arguments preserved in closure environment. - computedObj[key] = partial(fn, store) - computedCache[key] = computed(() => computedObj[key]()) - Object.defineProperty(store.getters, key, { - get: () => computedCache[key].value, - enumerable: true // for local getters - }) - }) - }) + // register the newly created effect scope to the store so that we can + // dispose the effects when this method runs again in the future. + store._scope = scope + registerGetters(store, Object.keys(store._wrappedGetters)) store._state = reactive({ data: state }) - // register the newly created effect scope to the store so that we can - // dispose the effects when this method runs again in the future. - store._scope = scope - // enable strict mode for new state if (store.strict) { enableStrictMode(store) @@ -86,6 +69,26 @@ export function resetStoreState (store, state, hot) { } } +export function registerGetters (store, getterKeys) { + const computedObj = {} + const computedCache = {} + + store._scope.run(() => { + getterKeys.forEach((key) => { + const fn = store._wrappedGetters[key] + // use computed to leverage its lazy-caching mechanism + // direct inline function use will lead to closure preserving oldState. + // using partial to return function with only arguments preserved in closure environment. + computedObj[key] = partial(fn, store) + computedCache[key] = computed(() => computedObj[key]()) + Object.defineProperty(store.getters, key, { + get: () => computedCache[key].value, + enumerable: true // for local getters + }) + }) + }) +} + export function installModule (store, rootState, path, module, hot) { const isRoot = !path.length const namespace = store._modules.getNamespace(path) diff --git a/src/store.js b/src/store.js index e22c74b5d..6fa83d576 100644 --- a/src/store.js +++ b/src/store.js @@ -9,7 +9,8 @@ import { installModule, resetStore, resetStoreState, - unifyObjectStyle + unifyObjectStyle, + registerGetters } from './store-util' export function createStore (options) { @@ -228,8 +229,10 @@ export class Store { this._modules.register(path, rawModule) installModule(this, this.state, path, this._modules.get(path), options.preserveState) - // reset store to update getters... - resetStoreState(this, this.state) + + const namespace = this._modules.getNamespace(path) + const getterKeys = Object.keys(rawModule.getters || {}).map((key) => namespace + key) + registerGetters(this, getterKeys) } unregisterModule (path) {