vuejs / vuex

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

Why aren't getters in modules nested by default? #679

Open snaptopixel opened 7 years ago

snaptopixel commented 7 years ago

The modules section in the docs states:

By default, actions, mutations and getters inside modules are still registered under the global namespace - this allows multiple modules to react to the same mutation/action type.

I'm left wondering why state properties are nested/namespaced by default but getters are not? It stands to reason that it would behave the same way since the getter(s) belong to the module...

const user = {
  state: {
    firstName: 'Bob',
    lastName: 'Dobalina'
  },
  getters: {
    fullName(state) {
      return `${state.firstName} ${state.lastName}`;
    }
  }
}

const App = {
  modules: {
    user
  }
}

App.state.user.firstName // Yay!
App.state.user.lastName // Yay!
App.getters.fullName // Aye?

I know namespaced provides this but it's too far reaching IMHO since it affects everything else. I'd rather be predictable with action and mutation names (especially since I'm using TypeScript) but allow getters to naturally go where they should, just like state properties do.

ktsn commented 7 years ago

I had described about that in the past. https://github.com/vuejs/vuex/issues/385#issuecomment-252910990

snaptopixel commented 7 years ago

Thanks @ktsn, to recap, you said:

We don't have to change the parts using getters even if we change module structure, because the getters completely independent on module structure.

I would think in most cases with modules, it's exactly the opposite. The point of modules (as I understand them) is to encapsulate functionality/data. This works great with state properties, but TBH it seems like getters in modules were not fully considered, making namespaced seem like a bit of an afterthought.

Let's take the example from the docs:

const store = new Vuex.Store({
  state: {
    todos: [
      { id: 1, text: '...', done: true },
      { id: 2, text: '...', done: false }
    ]
  },
  getters: {
    doneTodos: state => {
      return state.todos.filter(todo => todo.done)
    }
  }
})

If todos was a module, you would want doneTodos to be associated with it, not separate...

I understand the design, it just seems like it's not the right solution. If "getters are computeds for stores" they should behave as such. Just as you wouldn't want a computed on a component to be exposed on the root Vue instance.

twickstrom commented 6 years ago

I was hung up on this for far too long today... Using @snaptopixel example:

const user = {
  state: {
    firstName: 'Bob',
    lastName: 'Dobalina'
  },
  getters: {
    fullName(state) {
      return `${state.firstName} ${state.lastName}`;
    }
  }
}

const App = {
  modules: {
    user
  }
}

As long as namespaced is set this works as expected:

App.state.user.firstName
App.state.user.lastName

However, this does not work:

App.getters.user.fullName

To make it work you need to:

App.getters['user/fullName']

This does not seem consistent with the rest of the module implementation and I would propose being able to access the getters through the namespaced module like so:

App.getters.user.fullName
chearon commented 6 years ago

I agree it would be nice to have getters organized in a nested object like state. When you have a lot of modules, the getters being registered globally can be kind of a pain in the debugger.

I didn't really understand @ktsn's comment linked above:

Because getters have higher tolerance for changing module structure by separating with it. We don't have to change the parts using getters even if we change module structure, because the getters completely independent on module structure.

chearon commented 6 years ago

Is there any interest in this from core members? I spent a few hours on it and got nested getters working. The key is to leave the flat computed object alone and build a recursive state-like object off of that. Some of the other code could be cleaned up as a result of this too.

Really the only benefit of this is consistency with state and cosmetics. It looks nicer in the debugger. I will clean it up and open a PR if it would be considered, though.

twickstrom commented 6 years ago

I would be very interested. I’d also be curious of any benchmark data around your implementation vs what is in core.

twickstrom commented 6 years ago

@chearon looks like there is a pull request open for this that is passing all tests. Not sure where it sits on the roadmap.

You may want to check it out and drop in your thoughts in regards to the implementation.

https://github.com/vuejs/vuex/pull/1364

yanni4night commented 6 years ago

Interested too, the current namespaced getters are very hard to use, and ugly. The computed in Vue component is a good practice, but can not be nested.

chearon commented 6 years ago

computed can remain flat internally while Vuex exposes a nested structure though. That's what they did in #1364. They're still exposing the "ugly" ones with the / in them alongside the nested ones, though. I wish it did away with the whole getters["modulename/gettername"] thing entirely.

justinzpc commented 5 years ago

I agree it would be nice to have getters organized in a nested object like state. When you have a lot of modules, the getters being registered globally can be kind of a pain in the debugger.

I didn't really understand @ktsn's comment linked above:

Because getters have higher tolerance for changing module structure by separating with it. We don't have to change the parts using getters even if we change module structure, because the getters completely independent on module structure.

the pain in the debugger it was cause by developer, do not blame on the getters being registered globally, i prefer the globally over namespace, now the namespace make my program more complex, if developer could chose between the both that would be better!

Gameghostify commented 3 years ago

This is an old issue but still, I agree.

Why are getters inconsistent here? Could this be fixed with the next version of Vuex?

It feels weird being able to access state using store.foo.bar.baz.someProperty but having to use getters like store["foo/bar/baz/someGetter"]

Getting an update on this would be awesome