Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API change ideas #54

Closed
yyx990803 opened this issue Feb 15, 2016 · 13 comments
Closed

API change ideas #54

yyx990803 opened this issue Feb 15, 2016 · 13 comments

Comments

@yyx990803
Copy link
Member

Some breaking changes currently being considered.

Usage in components

Currently, all components that need to display store state or invoke store actions import the store directly. This makes the component definitions rely on a global singleton, and leads to the following drawbacks:

  1. A bit harder to test: the store needs to be mocked in order to test the component in isolation.
  2. Makes server side rendering hard, because ideally we want a fresh store instance for each request.

So the first thing we can do, is instead of directly importing the store in child components, we only import it at the root component and "inject" it into all child components:

// Vue.use(Vuex) teaches Vue instances to handle the `store` option
import store from '../store'
import App from '../components/App.vue'

// render root
new Vue({
  el: '#app',
  store, // provide store, and inject to all children
  components: { App }
})

Now, the store will automatically be available to all children as this.$store (similar to how this.$router is injected to all components enabled by vue-router). However, this makes things a bit more verbose and repetitive:

// example component after the change
export default {
  computed: {
    someState () {
      return this.$store.state.someState
    },
    computedState () {
      const { valueA, valueB } = this.$store.state
      return valueA + valueB
    }
  },
  methods: {
    someAction (...args) {
      this.$store.actions.someAction(...args)
    }
  }
}

So, the idea is introducing a vuex option:

import { someAction } from '../store/actions'

export default {
  vuex: {
    state: {
      someState: state => state.someState,
      computedState: ({ valueA, valueB }) => valueA + valueB
    },
    actions: {
      someAction, // becomes available as a method `this.someAction`
      // ad-hoc inline action
      inlineAction: ({ dispatch }, ...args) => dispatch('SOME_MUTATION', ...args)
    }
  }
}

Note what's happening here:

  1. vuex.state is basically sugar for computed properties. The difference is that the getters will get passed the store's state as the argument.
  2. vuex.actions is sugar for mapping actions to methods. Note **we are no longer exposing the actions on the store itself. Now the store is only concerned with state and mutations. Actions are simply functions that take store as the first argument and dispatch mutations on it. What the vuex.actions option does, is essentially binding raw action functions to this.$store, and then defining it as a directly callable method on the component. Here we are using the Object literal shorthand, so the method name will be the same (someAction). You can of course use a method name different from the imported function.

Now, why do we no longer expose actions on the store? Several reasons:

  1. Users have been asking about how to "namespace" the actions, because all actions share the same namespace on store.actions. By directly importing actions where needed, this problem no longer exists.
  2. This allows you to see where the action functions are defined right inside the component, thus allowing more flexibility in where to place them (instead of all inside a single actions.js file). For example you can split them up into store modules, and don't need to worry about not knowing which module an action is defined in.

Because actions are defined without directly depending on the store (instead takes it as an argument), importing them doesn't cause the singleton problem. When they are bound by vuex.actions internally, they just use whatever store the component has.

Module composition

Currently, module composition is a bit awkward:

import * as moduleA from './modules/a'
import * as moduleB from './modules/b'

export default new Vuex.Store({
  state: {
    a: moduleA.state,
    b: moduleB.state
  },
  mutations: [moduleA.mutations, moduleB.mutations]
})

This can get annoying when there are many modules. Also, mutations inside modules always get the whole state tree. This leads to:

  1. A module's mutation can actually mutate sub trees managed by other modules;
  2. A module's mutation needs knowledge of how modules are composed in order to "resolve" the sub tree it owns. This makes it hard to develop decoupled store modules.

Here's an idea for a better composition API:

// a module
const state = {
  count: 0
}

const mutations = {
  INCREMENT: state => state.count++, // receives only the sub tree owned by this module
  DECREMENT: state => state.count--
}

export default {
  state,
  mutations
}
// composing modules
import counter from './modules/counter'

const inlineModule =  {
  state: { n: 123 },
  mutations: { INCREMENT: state => state.n++ }
}

export default new Vuex.Store({
  state, // global state...
  mutations, // global mutations...
  modules: {
    counter, // adds and manages `store.state.counter`
    inline: inlineModule
  }
})

So modules essentially become "sub stores" that manages a sub tree of the state.

Unresolved questions

Actions still get the entire store. Which means if an action is shipped within a module, it has no way to know how to resolve its local state.

@blake-newman
Copy link
Member

I like the concept. However with experimentation on how to modularise the store it became more apparent to me that the store shouldn't modularise itself too much. When trying to modularise the concerns of a state to a particular feature/module it gave more inconsistencies. E.g. if an action was to affect multiple sub trees while modularised it could lead to a separation that didn't raise enough needs. This is why actions should be able to infer mutations on any part of the state.

The more we indulge the more we find that modularisation of the state was asking for too much for a flux architecture. I believe the API is correct for a robust state management system, however what could be improved mainly was how to implement solidly to a component. Using this.$state does add more verboseness, however this comes with benefits. So introducing a full fledged integration like this doesnt completely work for me.

Another great point raised is about SSR, this is difficult to implement with state as this is a natural flaw with SSR as the state is difficult to pass through SSR, especially in large applications. Could you explain more on how your changes will help SSR implementation?

I like Vuex in its current state, as it helps reduce repetitive code, clearly defines the barriers between mutations and actions and it keeps state management simple. Another great point is that the store acts as a single point of dependency. My fear with this API change is that the possibilities of the implementation details will multiply rapidly, creating more questions on how to correctly structure the store.

This is my initial reaction however i will delve further on this to see if i can sway myself into a different point of view.

@simplesmiler
Copy link
Member

Slightly tangent, but I've been using similar syntax to interact with redux:

var actions = require('./actions');
module.exports = {
  template: '<span @click="increment">{{ value }}<span>',
  redux: {
    select: function(state) {
      return { value: state.value };
    },
    actions: {
      increment: actions.increment,
    },
  },
};

Behind the scenes the instance subscribes to the store changes at created and unsubscribes at beforeDestroy. Upon changes, state is mapped to a slice using given select function, and then the slice is merged into vm (using logic that is similar to Object.assign). Actions are also getting bound to the store at created and are added to vm as if they were methods.

So far it's working great, but there's room for improvements, like using reselect and protecting store state from being "infected" with reactive getters and setters.

@sunabozu
Copy link

Totally support the new way of module composition.

But as for usage in components, I have a slightly different opinion. I believe a component should explicitly declare what data it uses, it should describe itself. So I'm against injecting a state to each component implicitly. Pieces of a state should be based by properties. However, I think the injection of actions is a very good idea. Currently I have to import actions in each component in order to notify my store about changes.

Still hope you will introduce some kind of namespaces for actions/mutations. It would be very, very helpful.

@yyx990803
Copy link
Member Author

@simplesmiler ha that looks extremely familiar.

@sunabozu the new syntax does declare what data a component uses. Note the vuex.state option is declaring the properties to "select" from the state and added to the current instance.

@bradstewart
Copy link

I really like the change to defining actions directly on components, I've got more than a few actions which only make sense for a specific component (e.g. login related stuff).

@rbvea
Copy link

rbvea commented Feb 23, 2016

I personally would love some sort of ability to automatically bind actions to a vue component. Lately I've been doing something like:

// SomeComponent.vue
<script>
import store from '../store';
const methods = store.actions;

const component = { ... };

// assign vuex actions to component methods
Object.assign(component, {
   methods
});

export default component;
</script>

It's just too bad there's no way to bind a decorator to an object.

Another idea is to do something similar to omniscient's component function and just assign the methods/state to the object that's passed in, then export that out. https://github.com/omniscientjs/omniscient

@simplesmiler
Copy link
Member

@rbvea well what you want can be done with mixins:

export default {
  mixins: [ { methods: store.actions } ],
  ...,
};

@rbvea
Copy link

rbvea commented Feb 23, 2016

@simplesmiler cool! didn't know that, could you do the same with a store being passed down as a prop?

@jbruni
Copy link
Contributor

jbruni commented Mar 1, 2016

I've just started playing with 0.4.0 branch, where there is no more support for store.actions. My first feeling relates to what @blake-newman said:

Another great point is that the store acts as a single point of dependency. My fear with this API change is that the possibilities of the implementation details will multiply rapidly, creating more questions on how to correctly structure the store.

Yep... Right now, I'm facing the issue of where to put my actions, since they can't belong to the store anymore. I can place them just anywhere now.

I understand this is a good "flexibility" feature. And I've seen the strong "actions" support for usage inside Vue components. In my case, I'm using actions from an Authentication service, which does not belong to any particular Vue component...

...while I was just calling store.actions.something(), now it seems I will need to create an additional layer... a separated "actions" module, which will import the store, and in my service I will import the actions module instead of the store. This may sound simple but in fact introduces a lot of unnecessary complexity.

Quoting @blake-newman again:

This is my initial reaction however i will delve further on this to see if i can sway myself into a different point of view.

Right now I am facing the "multiplied possibilities" + "questions on how to structure" phase, but maybe it is just a matter of moving forward a bit more...

@jbruni
Copy link
Contributor

jbruni commented Mar 1, 2016

I'm happy again. 😃 I moved forward towards a nice solution I'm enjoying. I am willing to share it. I hope I can get some free time to do so.

Indeed, we have multiplied possibilities and flexibility on how to structure... I was comfortable with the actions shorthands... I just re-created the little wrapper from 0.3.0...

I basically brought back the old good parts I missed, while I'm already enjoying the benefits of the newly added stuff, from the no-issues with vue-router, up to the new vuex component configuration option.

Very good, indeed! 👍

@yyx990803
Copy link
Member Author

@jbruni glad to hear that - this is exactly what is happening: in 0.4.0, actions and getters are just functions that you can organize and import anyway you like. While the docs/guide will recommend a way to organize them, you are free to develop your own organization patterns to fit your mental model better.

@blake-newman
Copy link
Member

@jbruni @yyx990803 I have also been experimenting with 0.4.0 and it feels right. I was more under the illusion that actions were still to be combined like mutations. Which caused complexity with naming, as you had to make sure that they had clear naming even if they did completely different actions but we're modularise to a specific file.

Modularising actions to a file gives flexibility, I enjoyed it so much Ive detached my actions from the store in my current project for my company in preparation to 0.4.0 release but more so that it makes more sense to not combine actions to the store.

@yyx990803
Copy link
Member Author

Shipped in 0.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants