From 8b9557026d290502cdcf5be93819838d730bff90 Mon Sep 17 00:00:00 2001 From: Dominik Pschenitschni Date: Mon, 23 Aug 2021 18:22:39 +0200 Subject: [PATCH 1/3] fix: vuex mutation violation from draggable The order of the list was originally updated by a sideeffect of the v-model. Since the data is mapped from the store this created a vuex mutation violation (data was edited by the v-model instead of a vuex mutation). In order to fix this I did the following: - create a computed for the filteredList -> No need to combine v-for and v-if for the list items - use value and @input instead of v-model for the draggable - create a new updateFilteredLists method that replaces the namespace with the old list with a one that includes a fresh list with the new order. - the list service returned the same model again. I got there some mutation problems aswell, so I return now a new model. vuex lists module - the toggleListFavorite action mutated the original list (by setting list.isFavorite). now it creates a new one to update it in the mutation instead. - something similar for the updateList action vuex namespaces module The old setNamespaceById used "for ... in" to iterate and get the list index. this is wrong! See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#array_iteration_and_for...in Quote: "Note: for...in should not be used to iterate over an Array where the index order is important." instead I find the index now with `Array.findIndex`. I also removed the line `namespace.lists = state.namespaces[n].lists` The reason was that the new namespace included the new ordered lists, so by overwriting it with the old one the modification wouldn't be saved. I hope this doesn't leave to errors somewhere else. --- src/components/home/navigation.vue | 54 ++++++++++++++++++++++-------- src/services/list.js | 5 +++ src/store/index.js | 2 ++ src/store/modules/lists.js | 36 ++++++++++++-------- src/store/modules/namespaces.js | 12 +++---- 5 files changed, 75 insertions(+), 34 deletions(-) diff --git a/src/components/home/navigation.vue b/src/components/home/navigation.vue index 82c4cdae7..f36911656 100644 --- a/src/components/home/navigation.vue +++ b/src/components/home/navigation.vue @@ -79,7 +79,8 @@ v-if="typeof listsVisible[n.id] !== 'undefined' ? listsVisible[n.id] : true" > -
  • @@ -167,13 +166,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', + menuActive: MENU_ACTIVE, + loading: state => state[LOADING] && state[LOADING_MODULE] === 'namespaces', + }), + filteredLists() { + return this.namespaces.map(({lists}) => lists.filter(item => !item.isArchived)) + }, + }, beforeCreate() { this.$store.dispatch('namespaces/loadNamespaces') .then(namespaces => { @@ -211,16 +215,38 @@ export default { toggleLists(namespaceId) { this.$set(this.listsVisible, namespaceId, !this.listsVisible[namespaceId] ?? false) }, + updateFilteredLists(namespace, filteredLists) { + // 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 filteredLists.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 listsFiltered = this.filteredLists[namespaceIndex] const list = listsFiltered[e.newIndex] const listBefore = listsFiltered[e.newIndex - 1] ?? null const listAfter = listsFiltered[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) }) diff --git a/src/services/list.js b/src/services/list.js index 22d73088a..5fd2fc3d2 100644 --- a/src/services/list.js +++ b/src/services/list.js @@ -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('') diff --git a/src/store/index.js b/src/store/index.js index df24ccd06..7f355b971 100644 --- a/src/store/index.js +++ b/src/store/index.js @@ -24,6 +24,8 @@ import ListService from '../services/list' Vue.use(Vuex) export const store = new Vuex.Store({ + // TODO: enable this to catch all vuex mutation violations + // strict: import.meta.env.DEV, modules: { config, auth, diff --git a/src/store/modules/lists.js b/src/store/modules/lists.js index fe66d76b7..214008014 100644 --- a/src/store/modules/lists.js +++ b/src/store/modules/lists.js @@ -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()) diff --git a/src/store/modules/namespaces.js b/src/store/modules/namespaces.js index 4dadd3475..65505db94 100644 --- a/src/store/modules/namespaces.js +++ b/src/store/modules/namespaces.js @@ -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) { -- 2.45.1 From 04c2877f42bee9d2b59da985cca3d6d816e1c36b Mon Sep 17 00:00:00 2001 From: Dominik Pschenitschni Date: Mon, 23 Aug 2021 19:04:09 +0200 Subject: [PATCH 2/3] chore: enable strict mode --- src/store/index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/store/index.js b/src/store/index.js index 7f355b971..921ff8649 100644 --- a/src/store/index.js +++ b/src/store/index.js @@ -24,8 +24,7 @@ import ListService from '../services/list' Vue.use(Vuex) export const store = new Vuex.Store({ - // TODO: enable this to catch all vuex mutation violations - // strict: import.meta.env.DEV, + strict: import.meta.env.DEV, modules: { config, auth, -- 2.45.1 From 8e32d23bffd41982bf5ebcf13fd24f7ce76fa276 Mon Sep 17 00:00:00 2001 From: Dominik Pschenitschni Date: Mon, 23 Aug 2021 20:33:51 +0200 Subject: [PATCH 3/3] chore: improve naming and explain fix in comment --- src/components/home/navigation.vue | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/components/home/navigation.vue b/src/components/home/navigation.vue index f36911656..af734f0b8 100644 --- a/src/components/home/navigation.vue +++ b/src/components/home/navigation.vue @@ -78,9 +78,13 @@ class="more-container" v-if="typeof listsVisible[n.id] !== 'undefined' ? listsVisible[n.id] : true" > +
  • state[LOADING] && state[LOADING_MODULE] === 'namespaces', }), - filteredLists() { + activeLists() { return this.namespaces.map(({lists}) => lists.filter(item => !item.isArchived)) }, }, @@ -215,7 +219,7 @@ export default { toggleLists(namespaceId) { this.$set(this.listsVisible, namespaceId, !this.listsVisible[namespaceId] ?? false) }, - updateFilteredLists(namespace, filteredLists) { + 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 @@ -223,7 +227,7 @@ export default { if (item.isArchived) { return item } - return filteredLists.shift() + return activeLists.shift() }) const newNamespace = { @@ -234,10 +238,10 @@ export default { this.$store.commit('namespaces/setNamespaceById', newNamespace) }, saveListPosition(e, namespaceIndex) { - const listsFiltered = this.filteredLists[namespaceIndex] - 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) const position = calculateItemPosition(listBefore !== null ? listBefore.position : null, listAfter !== null ? listAfter.position : null) -- 2.45.1