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: fix getters being destroyed on component destroy (#1878) #1883

Merged
merged 5 commits into from Oct 4, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions package.json
Expand Up @@ -52,7 +52,7 @@
},
"homepage": "https://github.com/vuejs/vuex#readme",
"peerDependencies": {
"vue": "^3.0.2"
"vue": "^3.2.0"
},
"dependencies": {
"@vue/devtools-api": "^6.0.0-beta.11"
Expand All @@ -65,7 +65,7 @@
"@rollup/plugin-node-resolve": "^13.0.0",
"@rollup/plugin-replace": "^2.4.2",
"@types/node": "^15.6.0",
"@vue/compiler-sfc": "^3.0.11",
"@vue/compiler-sfc": "^3.2.4",
"babel-jest": "^26.6.3",
"babel-loader": "^8.2.2",
"brotli": "^1.3.2",
Expand All @@ -89,8 +89,8 @@
"todomvc-app-css": "^2.4.1",
"typescript": "^4.2.4",
"vitepress": "^0.11.5",
"vue": "^3.0.11",
"vue-loader": "^16.2.0",
"vue": "^3.2.4",
"vue-loader": "^16.5.0",
"vue-style-loader": "^4.1.3",
"webpack": "^4.43.0",
"webpack-dev-middleware": "^3.7.2",
Expand Down
39 changes: 28 additions & 11 deletions src/store-util.js
@@ -1,4 +1,4 @@
import { reactive, watch } from 'vue'
import { reactive, computed, watch, effectScope } from 'vue'
import { forEachValue, isObject, isPromise, assert, partial } from './util'

export function genericSubscribe (fn, subs, options) {
Expand Down Expand Up @@ -29,30 +29,42 @@ export function resetStore (store, hot) {

export function resetStoreState (store, state, hot) {
const oldState = store._state
const oldScope = store._scope

// bind store public getters
store.getters = {}
// reset local getters cache
store._makeLocalGettersCache = Object.create(null)
const wrappedGetters = store._wrappedGetters
const computedObj = {}
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)
Object.defineProperty(store.getters, key, {
// TODO: use `computed` when it's possible. at the moment we can't due to
// https://github.com/vuejs/vuex/pull/1883
get: () => computedObj[key](),
enumerable: true // for local getters
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
})
})
})

store._state = reactive({
kiaking marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand All @@ -67,6 +79,11 @@ export function resetStoreState (store, state, hot) {
})
}
}

// dispose previously registered effect scope if there is one.
if (oldScope) {
oldScope.stop()
}
}

export function installModule (store, rootState, path, module, hot) {
Expand Down
6 changes: 6 additions & 0 deletions src/store.js
Expand Up @@ -39,6 +39,12 @@ export class Store {
this._modulesNamespaceMap = Object.create(null)
this._subscribers = []
this._makeLocalGettersCache = Object.create(null)

// EffectScope instance. when registering new getters, we wrap them inside
// EffectScope so that getters (computed) would not be destroyed on
// component unmount.
this._scope = null

this._devtools = devtools

// bind commit and dispatch to self
Expand Down
2 changes: 1 addition & 1 deletion test/helpers.js
Expand Up @@ -4,7 +4,7 @@ import puppeteer from 'puppeteer'
export function mount (store, component) {
const el = createElement()

component.render = () => {}
component.render = component.render ? component.render : () => {}
kiaking marked this conversation as resolved.
Show resolved Hide resolved

const app = createApp(component)

Expand Down
46 changes: 45 additions & 1 deletion test/unit/modules.spec.js
@@ -1,4 +1,5 @@
import { nextTick } from 'vue'
import { h, nextTick } from 'vue'
import { mount } from 'test/helpers'
import Vuex from '@/index'

const TEST = 'TEST'
Expand Down Expand Up @@ -124,6 +125,49 @@ describe('Modules', () => {
store.commit('a/foo')
expect(mutationSpy).toHaveBeenCalled()
})

it('should keep getters when component gets destroyed', async () => {
const store = new Vuex.Store()

const moduleA = {
namespaced: true,
state: () => ({ value: 1 }),
getters: {
getState: (state) => state.value
kiaking marked this conversation as resolved.
Show resolved Hide resolved
},
mutations: {
increment: (state) => { state.value++ }
}
}

const CompA = {
template: `<div />`,
created () {
this.$store.registerModule('moduleA', moduleA)
}
}

const CompB = {
template: `<div />`
}

const vm = mount(store, {
components: { CompA, CompB },
data: () => ({ show: 'a' }),
render () {
return this.show === 'a' ? h(CompA) : h(CompB)
}
})

expect(store.getters['moduleA/getState']).toBe(1)

vm.show = 'b'
await nextTick()

store.commit('moduleA/increment')

expect(store.getters['moduleA/getState']).toBe(2)
posva marked this conversation as resolved.
Show resolved Hide resolved
})
})

// #524
Expand Down