Better support for Vuex strict mode

[samburgers]

Hi,

I have a vue component, essentially like the below, allowing user to edit myCopy with v-model fields, then commit or reject the changes.

<template>
    <div v-if="myCopy">
        <input v-model="myCopy.title" type="text">
        <input v-model="myCopy.text" type="text">
        <button @click.prevent="commitCopy()">Save</button>
    </div>
</template>
<script>
import { mapGetters } from "vuex";
export default {
  computed: {
    ...mapGetters("someService", {
      myCopy: "getCopy"
    })
  },
  methods: {
    commitCopy() {
      // Commit Copy stuff
    },
  }
};
</script>

This works great until i start to edit one of the input fields, and I get this error:

[vuex] Do not mutate vuex store state outside mutation handlers.

I can prevent the issue, by changing my store.js file like this solution:

nuxt/nuxt#1917 (comment)

I don't feel like that is the right answer, and was just wondering if the getCopy getter should support a Two-way Computed Property like so:

https://vuex.vuejs.org/en/forms.html

Many thanks

[marshallswain]

Are you using Vuex in strict mode? If so, you might not be able to use the copy feature. I'm not certain that this would be the issue, though.

By the way, there's a new API coming that will allow multiple copies, you can check out the tests here: https://github.com/feathers-plus/feathers-vuex/blob/models/test/service-module/service-module.test.js#L29

[samburgers]

Thanks for the reply... and the incredible work with this package! Yep, thats right, if i disable strict mode it allows the mutations. Is that the ideal solution? Any other way around it? Cheers :)

[samburgers]

Nuxt ships with vuex strict mode by default, but its easy to disable in the store itself if anyone stumbles across this.
https://vuex.vuejs.org/en/strict.html

[marshallswain]

Uggh. I personally don't use strict mode, although I can see how it would be beneficial. It basically means you have to create a mutation for every change to an object. So, to update a user's age, instead of just pulling down a user record and doing user.age = 45, you have to create a setAge mutation and do commit('users/setAge({user, age: 45}) It's certainly useful to prevent errors during development, but you definitely don't want to ship an app in strict mode.

The official stance on vuex strict mode is that it requires much more work to implement it, period. The new API will open it up to allow better support for strict mode in a future release. The goal of feathers-vuex is to remove as much boilerplate code as possible. It does a great job of this outside of strict mode. In strict mode, the developer will have to write more custom mutations, which is just the nature of developing for strict mode.

The simplest way for somebody to support strict mode right now would be to create their own deep-cloned copy of a record as it comes out of the store, make changes to it, then use the updateItem mutation to update the store.

[marshallswain]

I'm really glad you opened this issue because it got me thinking more about strict mode. I thought that adhering to strict mode would definitely be a problem for the relational stuff that I'm working on. I just tested it out and it seems that, thankfully, all of my efforts will not be in vain!

So here's a preview of what's coming: Suppose you had an API that returns /todos data in the following format, where the todo record has an itemId and the item is populated as part of the response:

{
  id: 'todo-1',
  description: 'todo description',
  itemId: 'item-2',
  item: {
    id: 'item-2',
    test: true
  }
}

I've got it currently setup so that when this todo is inserted in the store, feathers-vuex will store the todo in the todos store, as you'd expect, but it will first automatically create the item in the items service store. When the todo instance is stored, it contains a reference to the item in the items store. I thought that due to the way strict mode is setup, it would have a fit with this, even if you used a mutation to make the changes. The good news is that it works without errors as long as you use a mutation!

Here's a mostly-full example:

// Setting up the store
this.store = new Vuex.Store({
  strict: true,
  plugins: [
    service('todos', {
      instanceDefaults: {
        id: null,
        description: '',
        isComplete: false,
        itemId: '',
        item: 'Item' // This sets up the relationship
      }
    }),
    service('items', {
      instanceDefaults: {
        test: false
      },
      mutations: {
        // Here's our custom mutation for updating the `test` boolean
        toggleTestBoolean (state, item) {
          item.test = !item.test
        }
      }
    })
  ]
})

// Now, assume the below code is in a component
const { Todo, Item } = this.$FeathersVuex

// When you create instances that already have an `id`, they get added to the store, automatically
const todo = new Todo({
  id: 'todo-1',
  description: 'todo description',
  itemId: 'item-2',
  // Because we setup the relationship, this gets added to the `items` store, since it also has an `id`
  item: {
    id: 'item-2',
    test: true
  }
})

const storedTodo = store.state.todos.keyedById['todo-1']
const storedItem = store.state.items.keyedById['item-2']

// Calling the mutation to change a record's property works fine in strict mode
store.commit('items/toggleTestBoolean', storedItem)

// Changing the property directly will throw errors in strict mode
todo.item.test = false

assert.equal(todo.item.test, false, 'the nested todo.item.test should be false')
assert.equal(storedTodo.item.test, false, 'the stored todo.item.test should be false')
assert.equal(storedItem.test, false, 'the stored item.test should be false')

So, if everything works out, this will all work in strict mode. It works in the tests, but I have to still test it out in a production app.

[marshallswain]

The pre-release works really well with strict mode, by default. I'm closing this issue because the pre-release is stable.

[FossPrime]

My issue was that some parent scope data was being changed outside the async block. The solution is to define anything you modify in the same block.

Bad when you have more than 1 page:

 fullFind: async function ({ commit, dispatch, getters, state }, args) {
      const sm = { p: { limit: 0, skip: 0, total: 1 } }
      const myArgs = args ? Object.assign({query: {}}, args) : {query: {}}
      myArgs.query = Object.assign({}, myArgs.query)
      
      while (sm.p.total > (sm.p.limit+sm.p.skip)) {
        myArgs.query.$skip = sm.p.skip + sm.p.limit // BAD
        sm.p = await dispatch('find', myArgs)
      }
     
      return getters.find(args)
    }

Good always:

fullFind: async function ({ commit, dispatch, getters, state }, args) {
      const sm = { p: { limit: 0, skip: 0, total: 1 } }
      
      while (sm.p.total > (sm.p.limit+sm.p.skip)) {
        const myArgs = args ? Object.assign({query: {}}, args) : {query: {}}
        myArgs.query = Object.assign({}, myArgs.query)
        myArgs.query.$skip = sm.p.skip + sm.p.limit
        sm.p = await dispatch('find', myArgs)
      }
     
      return getters.find(args)
    }

In this example sm.p doesn't trigger the error because i'm not modifying it's values, only a reference.