vuejs / vuex

🗃️ Centralized State Management for Vue.js.
https://vuex.vuejs.org
MIT License
28.42k stars 9.57k forks source link

[Feauture] Getters accessible in mutations #684

Closed karol-f closed 7 years ago

karol-f commented 7 years ago

It would be nice (from performance point of view) to allow accessing getters when performing mutations, e.g.:

mutations: {
    MUTATION_NAME(state, payload, { getters }) {
        getters.someExpensiveGetter.forEach((item) => item.mutated = 'test');
    }
}

Is it possible and good idea? What do you think?

LinusBorg commented 7 years ago

We already had that request recently, If I recall correctly.

My opinion then was, and still is, that actions already have access to getters and it should be their job to prepare the data necessary for a commit.

But I haven't seen performance as an argument in this discussion yet. In what scenarios do you see performance optimizations with getters in mutations?

karol-f commented 7 years ago

@LinusBorg I can understand your opinion, however I can't see how would it would work for all scenarios. Lets assume we have large collection state.collection = [...]. We have getter

filteredCollection({ state }) { 
    state.collection.filter((item) => item.status === 'open');
 }

How could I mutate all items in filteredCollection?

actions: {
    doSthWithFilteredCollection({ getters, commit }) {
        getters.filteredCollection.forEach((item, index) => {
            item.status = 'closed';
            commit('DO_SOMETHING', { item, index });
        })
    }
}

As you can see when passing index to commit function I will get index of already filtered filteredCollection so I can't easily update collection's item in DO_SOMETHING mutation.

Am I missing something?

My point is that I can't update items in large collection using caching power of getters. Every time when using mutation I have to do getter work(state.collection.filter((item) => item.status === 'open')) in case of getting only part of the collection, that I intend to change.

LinusBorg commented 7 years ago

An easy, immediate solution would be to pass the getter result into the mutation, and mutate that. It will be the same objects anyway.

But it doesn't feel totally right, I get that.

The broader topic here is that arrays of objects are generally problematic, performance-wise, in global stare systems like vuex or redux.

The general consensus seems to be that its best to keep the objects in a map object, keyed by id, and then an additional array of ids can be used if the order of objects is important.

An ordered array of objects is then easily achievable through a getter, if necessary.

karol-f commented 7 years ago

Thank you for the response. Regards.

sbusch commented 7 years ago

Sorry for hijacking this issue but I didn't want to open a new one...

I have this mutation:

mutations: {
  // ...
  add_product_to_cart (state, product) {
    if (state.cart.products.indexOf(product) > -1) {
      return
    }
    state.cart.products.push(product)
  },
  // ...
}

But in other places I alreay need the following getter ...

getters: {
  // ....
  is_product_in_cart (state, getters) {
    return (product) => {
      return (state.cart.products.indexOf(product) > -1)
    }
  },
  // ...

... which I would also like to re-use in the mutation. Possible?

LinusBorg commented 7 years ago
  1. This is a closed issue
  2. Issues are reserved for bugs and feature request. Support @ forum.vuejs.org or the gitter chat.
bbugh commented 6 years ago

If anyone else finds this looking for an answer, you can just use a function outside of the mutations definition:

function is_product_in_cart (state, product) {
  return state.cart.products.indexOf(product) > -1
}

export default {
  mutations: {
    add_product_to_cart: function (state, product) {
      if (is_product_in_cart(state, product)) {
        return
      }
      state.cart.products.push(product)
    }
  },

  getters: {
    is_product_in_cart: (state) => (product) => is_product_in_cart(state, product)
  }
}
davidm-public commented 6 years ago

@LinusBorg getters are available for actions, but.... for synchronous changes to the database, actions are not needed, correct? so in those cases ie: mutations without actions, it would be useful to have the getters (for local and root state)

Note: ommiting actions and going straight to mutations:

a) significantly reduces the boilerplate, and

b) removes the need to pass the mutation name by string (a major smell, reminds one of the issues with Angular v1 !)

augnustin commented 4 years ago

:+1: here.

As a consequence, instead of having one mutation per mutation type, to avoid too much repetition, I'm writing:

export const state = () => ([])

export const mutations {
  mutateObject(state, object, fields) {
    Object.assign(state.find(o => o === object), fields);
  }
}

export const actions {
  shouldHaveBeenAMutation({commit, getters}, field) {
    commit('mutateObject', getters.currentObject, {field})
  }
}

And suddenly all the cleanness is gone ...

Besides, I don't understand why vuex doesn't consider objects as immutable like redux does, this would be much less confusing. But that's another topic...

jcalfee commented 2 years ago

It would be good to mention getters on https://vuex.vuejs.org/guide/mutations.html .. It is something everyone has to be aware of sooner or later.

ragnese commented 2 years ago

If anyone else finds this looking for an answer, you can just use a function outside of the mutations definition:

function is_product_in_cart (state, product) {
  return state.cart.products.indexOf(product) > -1
}

export default {
  mutations: {
    add_product_to_cart: function (state, product) {
      if (is_product_in_cart(state, product)) {
        return
      }
      state.cart.products.push(product)
    }
  },

  getters: {
    is_product_in_cart: (state) => (product) => is_product_in_cart(state, product)
  }
}

The reason this isn't ideal is because getters cache their results. A mutation should be able to access the cache result instead of needing to recompute it every time.

@LinusBorg

Apologies for commenting on an already closed issue, but none of the other comments I've seen have addressed my concern with regard to Getters being available in Mutations.

Like the initial feature request here, I see this as a performance issue/improvement.

In my module, I have a Getter that computes a (let's pretend) potentially expensive value by inspecting the state and looping over a bunch of data in an array.

I have a mutation to update the state, but, I want to check the payload against this computed value to make sure that the mutation is valid. If it's not, I throw an Error, rather than allow the state to be updated with invalid data.

Because of this "safety concern", there's no way for an Action to replace this mechanism. An Action would have to call a Mutation, which means that the Mutation would obviously have to exist. If the validation is does in the Action, that means it's not being done in the Mutation. As far as I know, you can't really have "private" Mutations in your modules, so I'd be exposing the unsafe Mutation and myself, or my colleagues, may some day call it by mistake.

Recomputing the validation data on each call to the Mutation is unnecessarily expensive.

Furthermore, I don't find any compelling philosophical reason for Mutations to not be able to access Getters. It's just data that could just as well have been part of the state, itself.

olavfosse commented 2 years ago

bump