fix: vuex mutation violation from draggable #674

dpschen · 2021-08-23T16:28:35Z

dpschen commented

2021-08-23 16:28:35 +00:00

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.

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.

konrad reviewed 2021-08-23 16:56:07 +00:00

src/components/home/navigation.vue Outdated

						
				@ -81,2 +81,3 @@

									<draggable

										v-model="n.lists"

										:value="filteredLists[nk]"

										@input="(lists) => updateFilteredLists(n, lists)"

konrad commented

2021-08-23 16:48:43 +00:00

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

dpschen commented

2021-08-23 17:16:15 +00:00

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.

dpschen commented

2021-08-23 17:36:52 +00:00

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 :/

konrad commented

2021-08-23 17:47:04 +00:00

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?

👍 1

dpschen marked this conversation as resolved

src/components/home/navigation.vue Outdated

						
				@ -177,0 +174,4 @@

							menuActive: MENU_ACTIVE,

							loading: state => state[LOADING] && state[LOADING_MODULE] === 'namespaces',

						}),

						filteredLists() {

konrad commented

2021-08-23 16:50:41 +00:00

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?

dpschen commented

2021-08-23 17:06:52 +00:00

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`

konrad commented

2021-08-23 17:48:24 +00:00

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.

👍 1

dpschen marked this conversation as resolved

src/store/index.js Outdated

						
				@ -25,2 +25,4 @@

				export const store = new Vuex.Store({

					// TODO: enable this to catch all vuex mutation violations

					// strict: import.meta.env.DEV,

konrad commented

2021-08-23 16:55:21 +00:00

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.

👍 1

dpschen marked this conversation as resolved

konrad commented

2021-08-23 16:56:53 +00:00

I wonder how the changes affect the performance of it - replacing whole models is probably i bit more expensive in terms of cpu ressouces, but probably not noticable.

konrad commented

2021-08-23 16:57:56 +00:00

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

Thanks for pointing that out, I did not know that.

> 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 Thanks for pointing that out, I did not know that.

👍 1

dpschen commented

2021-08-23 17:18:04 +00:00

I wonder how the changes affect the performance of it - replacing whole models is probably i bit more expensive in terms of cpu ressouces, but probably not noticable.

This shouldn't be a problem. Replacing large chunks is a common thing to do!
Most of the time it's more important, because we let the browser / vue / vuex do it's magic instead of trying to build update code that is as performant.
Vue won't replace the whole object. Instead it will diff the difference.

> I wonder how the changes affect the performance of it - replacing whole models is probably i bit more expensive in terms of cpu ressouces, but probably not noticable. This shouldn't be a problem. Replacing large chunks is a common thing to do! Most of the time it's more important, because we let the browser / vue / vuex do it's magic instead of trying to build update code that is as performant. Vue won't replace the whole object. Instead it will diff the difference.

konrad commented

2021-08-23 17:48:59 +00:00

Most of the time it's more important, because we let the browser / vue / vuex do it's magic instead of trying to build update code that is as performant.
Vue won't replace the whole object. Instead it will diff the difference.

I guess this is fine then. Thanks for the explanation.

> Most of the time it's more important, because we let the browser / vue / vuex do it's magic instead of trying to build update code that is as performant. Vue won't replace the whole object. Instead it will diff the difference. I guess this is fine then. Thanks for the explanation.

dpschen commented

2021-08-23 18:40:43 +00:00

I think it should now all be resolved :)

dpschen force-pushed feature/fix-mutation-violation from 864f5ced76 to 8e32d23bff

2021-08-23 19:01:38 +00:00

Compare

konrad approved these changes 2021-08-23 19:24:35 +00:00

konrad merged commit 0a8505f53c into main

2021-08-23 19:24:52 +00:00

konrad referenced this issue from a commit

2021-08-23 19:24:52 +00:00

fix: vuex mutation violation from draggable (#674)