fix: vuex mutation violation from draggable #674

Merged
konrad merged 3 commits from dpschen/frontend:feature/fix-mutation-violation into main 2021-08-23 19:24:52 +00:00
5 changed files with 81 additions and 37 deletions

View File

@ -78,8 +78,13 @@
class="more-container"
v-if="typeof listsVisible[n.id] !== 'undefined' ? listsVisible[n.id] : true"
>
<!--
NOTE: a v-model / computed setter is not possible, since the updateActiveLists function
triggered by the change needs to have access to the current namespace
dpschen marked this conversation as resolved Outdated

Could you achieve the same effect with a setter for the computed property?

Could you achieve the same effect with a setter for the computed property?

In theory yes. But in order to do so it would need to know the current namespace.
The name was inspired by your listsFiltered in the saveListPosition function.

In theory yes. But in order to do so it would need to know the current namespace. The name was inspired by your `listsFiltered` in the `saveListPosition` function.
v-model="activeLists(nk)"
activeLists() {
	return (namespace) => ({
		get() {
			return namespace.lists.filter(item => !item.isArchived)
		},
		set(activeLists) {
			this.updateActiveLists(namespace, activeLists)
		},
	)
},

Does not work :/

```html v-model="activeLists(nk)" ``` ```js activeLists() { return (namespace) => ({ get() { return namespace.lists.filter(item => !item.isArchived) }, set(activeLists) { this.updateActiveLists(namespace, activeLists) }, ) }, ``` Does not work :/

I see, makes sense then. Could you add a comment about this then so that future readers of the code know why there isn't a setter?

I see, makes sense then. Could you add a comment about this then so that future readers of the code know why there isn't a setter?
-->
<draggable
v-model="n.lists"
:value="activeLists[nk]"
@input="(lists) => updateActiveLists(n, lists)"
:group="`namespace-${n.id}-lists`"
@start="() => drag = true"
@end="e => saveListPosition(e, nk)"
@ -94,11 +99,9 @@
tag="ul"
class="menu-list can-be-hidden"
>
<!-- eslint-disable vue/no-use-v-if-with-v-for,vue/no-confusing-v-for-v-if -->
<li
v-for="l in n.lists"
v-for="l in activeLists[nk]"
:key="l.id"
v-if="!l.isArchived"
class="loader-container"
:class="{'is-loading': listUpdating[l.id]}"
>
@ -167,13 +170,18 @@ export default {
NamespaceSettingsDropdown,
draggable,
},
computed: mapState({
namespaces: state => state.namespaces.namespaces.filter(n => !n.isArchived),
currentList: CURRENT_LIST,
background: 'background',
menuActive: MENU_ACTIVE,
loading: state => state[LOADING] && state[LOADING_MODULE] === 'namespaces',
}),
computed: {
...mapState({
namespaces: state => state.namespaces.namespaces.filter(n => !n.isArchived),
currentList: CURRENT_LIST,
background: 'background',
dpschen marked this conversation as resolved Outdated

Maybe this should be called something like namespacesWithFilteredLists (I know, long) to make it clear this is not just an array of lists but an array of namespaces with filtered lists?

Maybe this should be called something like `namespacesWithFilteredLists` (I know, long) to make it clear this is not just an array of lists but an array of namespaces with filtered lists?

But the computed is just an array of the lists. No namespaces.

How about activeLists
In the sense of !archived === active

But the computed is just an array of the lists. No namespaces. How about `activeLists` In the sense of `!archived === active`

But the computed is just an array of the lists. No namespaces.

My bad, I thought it was everything.

How about activeLists
In the sense of !archived === active

I think that would work great.

> But the computed is just an array of the lists. No namespaces. My bad, I thought it was everything. > How about activeLists In the sense of !archived === active I think that would work great.
menuActive: MENU_ACTIVE,
loading: state => state[LOADING] && state[LOADING_MODULE] === 'namespaces',
}),
activeLists() {
return this.namespaces.map(({lists}) => lists.filter(item => !item.isArchived))
},
},
beforeCreate() {
this.$store.dispatch('namespaces/loadNamespaces')
.then(namespaces => {
@ -211,16 +219,38 @@ export default {
toggleLists(namespaceId) {
this.$set(this.listsVisible, namespaceId, !this.listsVisible[namespaceId] ?? false)
},
updateActiveLists(namespace, activeLists) {
// this is a bit hacky: since we do have to filter out the archived items from the list
// for vue draggable updating it is not as simple as replacing it.
// instead we iterate over the non archived items in the old list and replace them with the ones in their new order
const lists = namespace.lists.map((item) => {
if (item.isArchived) {
return item
}
return activeLists.shift()
})
const newNamespace = {
...namespace,
lists,
}
this.$store.commit('namespaces/setNamespaceById', newNamespace)
},
saveListPosition(e, namespaceIndex) {
const listsFiltered = this.namespaces[namespaceIndex].lists.filter(l => !l.isArchived)
const list = listsFiltered[e.newIndex]
const listBefore = listsFiltered[e.newIndex - 1] ?? null
const listAfter = listsFiltered[e.newIndex + 1] ?? null
const listsActive = this.activeLists[namespaceIndex]
const list = listsActive[e.newIndex]
const listBefore = listsActive[e.newIndex - 1] ?? null
const listAfter = listsActive[e.newIndex + 1] ?? null
this.$set(this.listUpdating, list.id, true)
list.position = calculateItemPosition(listBefore !== null ? listBefore.position : null, listAfter !== null ? listAfter.position : null)
const position = calculateItemPosition(listBefore !== null ? listBefore.position : null, listAfter !== null ? listAfter.position : null)
this.$store.dispatch('lists/updateList', list)
// create a copy of the list in order to not violate vuex mutations
this.$store.dispatch('lists/updateList', {
...list,
position,
})
.catch(e => {
this.error(e)
})

View File

@ -39,6 +39,11 @@ export default class ListService extends AbstractService {
return list
}
update(model) {
const newModel = { ... model }
return super.update(newModel)
}
background(list) {
if (list.background === null) {
return Promise.resolve('')

View File

@ -24,6 +24,7 @@ import ListService from '../services/list'
Vue.use(Vuex)
export const store = new Vuex.Store({
strict: import.meta.env.DEV,
modules: {
dpschen marked this conversation as resolved Outdated

Please remove the TODO and either just uncomment it so it is always enabled when in dev mode or remove it entirely.

Please remove the `TODO` and either just uncomment it so it is always enabled when in dev mode or remove it entirely.
config,
auth,

View File

@ -38,9 +38,10 @@ export default {
},
actions: {
toggleListFavorite(ctx, list) {
list.isFavorite = !list.isFavorite
return ctx.dispatch('updateList', list)
return ctx.dispatch('updateList', {
...list,
isFavorite: !list.isFavorite,
})
},
createList(ctx, list) {
const cancel = setLoading(ctx, 'lists')
@ -61,24 +62,31 @@ export default {
const listService = new ListService()
return listService.update(list)
.then(r => {
ctx.commit('setList', r)
ctx.commit('namespaces/setListInNamespaceById', r, {root: true})
if (r.isFavorite) {
r.namespaceId = FavoriteListsNamespace
ctx.commit('namespaces/addListToNamespace', r, {root: true})
.then(() => {
ctx.commit('setList', list)
ctx.commit('namespaces/setListInNamespaceById', list, {root: true})
// the returned list from listService.update is the same!
// in order to not validate vuex mutations we have to create a new copy
const newList = {
...list,
namespaceId: FavoriteListsNamespace,
}
if (list.isFavorite) {
ctx.commit('namespaces/addListToNamespace', newList, {root: true})
} else {
r.namespaceId = FavoriteListsNamespace
ctx.commit('namespaces/removeListFromNamespaceById', r, {root: true})
ctx.commit('namespaces/removeListFromNamespaceById', newList, {root: true})
}
ctx.dispatch('namespaces/loadNamespacesIfFavoritesDontExist', null, {root: true})
ctx.dispatch('namespaces/removeFavoritesNamespaceIfEmpty', null, {root: true})
return Promise.resolve(r)
return Promise.resolve(newList)
})
.catch(e => {
// Reset the list state to the initial one to avoid confusion for the user
list.isFavorite = !list.isFavorite
ctx.commit('setList', list)
ctx.commit('setList', {
...list,
isFavorite: !list.isFavorite,
})
return Promise.reject(e)
})
.finally(() => cancel())

View File

@ -13,13 +13,13 @@ export default {
state.namespaces = namespaces
},
setNamespaceById(state, namespace) {
for (const n in state.namespaces) {
if (state.namespaces[n].id === namespace.id) {
namespace.lists = state.namespaces[n].lists
Vue.set(state.namespaces, n, namespace)
return
}
const namespaceIndex = state.namespaces.findIndex(n => n.id === namespace.id)
if (namespaceIndex === -1) {
return
}
Vue.set(state.namespaces, namespaceIndex, namespace)
},
setListInNamespaceById(state, list) {
for (const n in state.namespaces) {