Mutation handlers need some serious re-evaluation 🐒

aleksey-hoffman commented 3 years ago

What problem does this feature solve?

Hey guys, can we discuss Vuex mutation handlers - arguably the most nonsensical thing in Vuex?

I've been developing a big complex app for 2+ years with Vue and I still haven't found good reasons to keep using mutation handlers. According to Vuex docs and my experience, the only thing they do is make debugging a little bit easier in some situations, while over-complicating the codebase and making development process undoubtedly a lot more annoying for no good reason.

Examples:

Compare the 2 examples demonstrated below, look at how much less code it takes to open a dialog and to change some of its properties by using the 1st method, compared to the 2nd method.

Method 1: direct mutation

This method achieves the same thing as the 2nd method, but requires hundreds of times less code (when app has a lot of components), which in itself makes code cleaner and less susceptible to bugs, which results in much more enjoyable development experience.

`App.vue`

<v-btn @click="createNewItem({ type: 'file', extension: 'txt' })">
  Create new item
</v-btn>

watch: {
  // Creating a global store watcher that handles actions, required for specific store properties
  dialogs () {
    handler (value) {
      const propertyShouldBeWrittenToDrive = ...
      if (propertyShouldBeWrittenToDrive) {
        this.$store.dispatch('WRITE_TO_DRIVE', propertyShouldBeWrittenToDrive)
      }
    },
    deep: true
  }
},
methods: {
  createNewItem (params) {
    // Set dialog data
    this.$store.state.dialogs.createNewItemDialog.data = { 
      ...this.$store.state.dialogs.createNewItemDialog.data,
      ...params 
    }
    // Open dialog
    this.$store.state.dialogs.createNewItemDialog.isOpened = true
  }
}

`Dialogs.vue`

<v-dialog v-model="$store.state.dialogs.createNewItemDialog.isOpened">
  <div class="title">
    Create new {{$store.state.dialogs.createNewItemDialog.data.type}}
  </div>
  <v-text-field
    v-model="createNewItemDialog.data.name"
  />
  <v-btn @click="$store.state.dialogs.createNewItemDialog.isOpened = false">
    close dialog
  </v-btn>
</v-dialog>

// NO COMPUTED PROPERTIES NEEDED

`store.js`

state = {
  dialogs: {
    createNewItemDialog: {
      value: false,
      data: {
        type: 'file',
        name: '',
        extension: ''
      }
    } 
  }
}

// NO MUTATIONS AND ACTIONS NEEDED

I know this method might, in theory, make it more difficult to track down the source of errors, but in practice, if there's only 3-5 functions that change a specific state property, it's pretty easy to determine which one of them caused the error by following the event logic. Mutations are always caused by some sort of known event (button press, input change, etc), so when an error appears during a specific event, it's not that difficult to track down the function that caused it, without using mutation handlers anyway.

Method 2: mutation handlers

This method is considered to be "the best practice" just because it improves debugging, while requiring a ton more work. Here's all the code you will need to set up the same dialog using this method.

This example is written with the official Vuex guidelines in mind (create getters, setters, create a separate action and mutation for each property, etc) to additionally demonstrate the absurdity of the current Vuex implementation.

`App.vue`

<v-btn @click="createNewItem({ type: 'file', extension: 'txt' })">
  Create new item
</v-btn>

computed: {
  createNewItemDialogIsOpened: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.isOpened
    },
    set (value) {
      this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_IS_OPENED', value)
    }
  },
  createNewItemDialogData: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.data
    },
    set (value) {
      this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA', value)
    }
  },
  // 100 more similar computed properties for all the baseline properties of the other 50 dialogs
  // ... {1,000 lines of code} ...
},
methods: {
  createNewItem (params) {
    // Set dialog data
    this.createNewItemDialogData = { ...this.createNewItemDialogData, ...params }
    // Open dialog
    this.createNewItemDialogIsOpened = true
  }
}

`Dialogs.vue`

<v-dialog v-model="createNewItemDialogIsOpened">
  <div class="title">
    Create new {{createNewItemDialogDataType}}
  </div>
  <v-text-field
    v-model="createNewItemDialogDataName"
  />
  <v-text-field
    v-model="createNewItemDialogDataExtension"
  />
  <v-btn @click="createNewItemDialogIsOpened = false">
    close dialog
  </v-btn>
</v-dialog>

computed: {
  createNewItemDialogIsOpened: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.isOpened
    },
    set (value) {
      this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_IS_OPENED', value)
    }
  },
  createNewItemDialogDataType: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.data.type
    },
    set (value) {
      this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_TYPE', value)    
    }
  },
  createNewItemDialogDataName: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.data.name
    },
    set (value) {
      this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_NAME', value)
    }
  },
  createNewItemDialogDataExtension: {
    get () {
      return this.$store.state.dialogs.createNewItemDialog.data.extension
    },
    set (value) {
       this.$store.dispatch('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_EXTENSION', value)
    }
  },
  // 300 more similar computed properties for all the properties of the other 50 dialogs
  // ... {3,000 lines of code} ...
}

`store.js`

const state = {
  dialogs: {
    createNewItemDialog: {
      value: false,
      data: {
        type: 'file',
        name: '',
        extension: ''
      }
    } 
  }
}

mutations: {
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_IS_OPENED (state, value) {
    state.dialogs.createNewItemDialog.isOpened = value
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA (state, value) {
    state.dialogs.createNewItemDialog.data = value
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_TYPE (state, value) {
    state.dialogs.createNewItemDialog.data.type = value
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_NAME (state, value) {
    state.dialogs.createNewItemDialog.data.name= value
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_EXTENSION (state, value) {
    state.dialogs.createNewItemDialog.data.extension= value
  },
  // 5000 more similar Vuex mutations for all the properties of the other 50 dialogs and other components
  // ... {50,000 lines of code} ...
},
actions: {
  WRITE_TO_DRIVE (state, params) {
    ...
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_IS_OPENED ({state, dispatch}, value) {
    commit('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_IS_OPENED', value)
    dispatch('WRITE_TO_DRIVE', value)
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA ({state, dispatch}, value) {
    commit('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA', value)
  },
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_TYPE ({state, dispatch}, value) {
    commit('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_TYPE ', value)
  }, 
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_NAME ({state, dispatch}, value) {
    commit('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_NAME ', value)
  }, 
  SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_EXTENSION ({state, dispatch}, value) {
    commit('SET_DIALOGS_CREATE_NEW_ITEM_DIALOG_DATA_EXTENSION', value)
  }, 
  // 5000 more similar Vuex actions for all the properties of the other 50 dialogs and other components
  // ... {50,000 lines of code} ...
}

And you have to do this for every single component of the app.

Add to that the inability to mutate properties from within a Vuex action and the fact that direct mutations are 10 - 500 times faster than the mutation handlers in cases where properties is mutated rapidly (once every few milliseconds), e.g. a slider without a debouncer / throttle, and it becomes clear that this insanity needs a re-evaluation.

In the real code, I managed to reduce the amount of computed properties by generating them dynamically for every nested property in beforeCreated hook of every component, and reduced the amount of mutations by creating a single generalized custom mutation function that sets specified deep properties automatically from the specified 'dot.notation.property.path' string. But if this kind of heavy lifting is supposed be implemented manually by every developer for every new project, Vuex shouldn't really be considered a library then.

What does the proposed API look like?

Anything but this. Surely there is a better / smarter / simpler way to do traceable mutations.

posva commented 3 years ago

This is why we are reconsidering mutations for Vuex 5, so please wait for the RFC to give your feedback. In the meantime, you can create your own Store with composition API and avoid mutations completely. You could also take a look at Pinia Store which does not have mutations, but ultimately, Vuex 5 will bring an API that isn't as verbose as with mutations

Also note that with the composition API, you can design your own map* helpers to create computed properties that that be read and written while still using Vuex 3/4

aleksey-hoffman commented 3 years ago

@posva thank you for the suggestions, I'll look into this.

What are the major purposes of mutation handlers anyway? Is it just debugging? What if all components just ignore mutation handlers and mutate all store properties directly, like shown in the method 1 above - will we only loose better debugging capabilities and that's it?

adavie1 commented 2 years ago

Hear hear!

Vuex doesn't follow the KISS principle.

Too much code write for every store - .mutations, actions, getters, ..mapGetters everywhere, plus computed methods...

Also getters take too much code!

THIS DOESN'T WORK - From the main page.

getters: {
  // ...
  getTodoById: (state) => (id) => {
    return state.todos.find(todo => todo.id === id)
  }
}

store.getters.getTodoById(2) // -> { id: 2, text: '...', done: false }

vuejs / vuex