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

Interact with modules more like components, perhaps even models? #359

Closed
Sarke opened this issue Oct 2, 2016 · 11 comments
Closed

Interact with modules more like components, perhaps even models? #359

Sarke opened this issue Oct 2, 2016 · 11 comments

Comments

@Sarke
Copy link

Sarke commented Oct 2, 2016

First off, congrats on 2.0!

I like how you can now include getters and actions in the modules, that's a good improvements. However, I think it still could be better. I know Vuex tries to keep the objects simple to work with the mutation tracking and such, but I am used to working with object models and they just make so much sense to me.

Compare how a Vue Component can have a computed value and a real data value. If there is a computed getter then it will be used, if not the data value will be used. I feel something similar would make sense for Vuex modules.

So, imagine we could do something like this?

let user = this.$store.model.user

user.id
// equal to this.$store.state.user.id

user.isAdmin
// equal to this.$store.getters.user_isAdmin

user.login(credentials)
// equal to this.$store.dispatch('user_login', credentials)

user.name = newName
// equal to this.$store.commit('USER_SET_NAME', newName)

The last one, the setter that does a commit, I feel would have the most opposition to implementation, as it hides the whole mutation flow a bit and is the more vague of the four. However, I think these would make a big usability improvement for the Vuex store interactions, and it would be something people would be free to use (or not to use) as it doesn't change how anything else works.

The content of the user.js module would be something like this:

const state = {
    id: 0,
    admin: false,
}

const mutations = {
    [USER_SET_NAME](state, newName) {
        state.name = newName
    },
}

const getters = {
    isAdmin() {
        return state.admin
    },
}

const actions = {
    login({ commit }, credentials) {
        // ... logic
    },
}

export default {
    state,
    mutations,
    getters,
    actions,
    prefix: true, // prefixes actions and getters with 'user_'
    activateModels: true, // makes the store.models available
}

The first option prefix is a way of namespacing the store.getters and store.actions without having to keep repeating that in the module. I am not a fan of how modules all share the same getters and actions names and can overwrite each other, since I (obviously) feel they should be compartmentalized like the components (and models).

The second option 'activateModels' might be a good idea if there are any negative issues with loading the modules into a store.models interface.

Perhaps both options should be global and not per-module options...

I'm very happy with what Vue and Vuex is, and I am looking forward to your thoughts and discussing the suggestion.

EDIT: I'm not sure how this would work with collections of models. Perhaps specifying the id in a third option (defaulting to id) would allow for loading a model from a collection, e.g this.$store.model.user[1] or this.$store.loadModel('user', 1)?

@ktsn
Copy link
Member

ktsn commented Oct 3, 2016

Thank you for your suggestion.
What is the motivation of your suggestion?
It seems that map... helpers can achieve similar thing.

BTW, automatic prefixing was suggested on first design phase of v2.0 but it was rejected by community discussion.

@ktsn ktsn added the proposal label Oct 3, 2016
@Sarke
Copy link
Author

Sarke commented Oct 3, 2016

What is the motivation of your suggestion?

Like I said, I'm used to, and I'm sure many others are as well, dealing with data as a model entity. Just like an ORM/ActiveRecord let's you deal with an object instead of the database. The entity (e.g. User) is the focal point, not the vuex store.

The model pattern is used by the Vue Components (view-model), so I don't understand why we can't treat the store modules as models or Components.

It seems that map... helpers can achieve similar thing.

But you still run into the issue with namespacing (which exists for a reason). Having to prefix every action, getter, and mutation in a store module seems very much against DRY.

BTW, automatic prefixing was suggested on first design phase of v2.0 but it was rejected by community discussion.

That's why I think an option to enable prefixing would be a good solution,

@ktsn
Copy link
Member

ktsn commented Oct 4, 2016

I think we should separate the namespace issue and model feature.

Namespace

As for auto-namespacing, it suffers the flexibility of store usage as discussed in the past. For example, we sometimes trigger multiple actions and mutations by dispatching once. This demand is reasonable because Vuex actions and mutations are rely on event emitter pattern.

If we don't want to write same prefix repeatedly, we can make our own helpers for it as I described in #335.

But I also think that if we can decide module's prefix explicitly, it would retain naming flexibility and also reduce the cost of writing module definition. So, how about prefix: string option instead of prefix: boolean?

export default {
  prefix: 'user/',

  // or
  suffix: '...',

  // or maybe
  typeTransformer: name => /* ... */,

  // module assets
  state: { ... },
  getters: {
    isAdmin () { ... } // -> getters['user/isAdmin']
  },
  actions: {
    login () { ... } // -> dispatch('user/login')
  },
  mutations: {
    setName () { ... } // -> commit('user/setName')
  }
}

Model feature

About model feature, IMO, mapping a module as a model object would be bad idea because:

  • Actions and mutations can affect other modules and also other modules can dispatch or commit them.
  • The model interface is not intuitive for the actual behavior as it triggers some events rather than just assign values to properties.
  • We cannot use same name among state, getters, actions and mutations since they will assign on same object. This is inconsistent restriction with current store API.

However, I like your "shortcut" idea as omitting prefix under some specific context. How would you feel about creating sub store instead of model? We can omit annoying prefix on a sub store.

export default {
  subStore: 'user',
  prefix: 'user/'

  // module assets
  state: {
    id: 0,
    admin: false,
  },
  getters: {
    isAdmin: state => state.admin
  },
  actions: {
    login () { ... }
  },
  mutations: {
    setName () { ... },
  }
}

// we can omit prefix on sub store
const user = store.subStores.user
user.state.id // -> 0
user.state.admin // -> false
user.getters.isAdmin // -> store.getters['user/isAdmin'] -> false
user.dispatch('login', credentials) // store.dispatch('user/login', credentials)
user.commit('setName', newName) // store.commit('user/setName', newName)

For component binding, helpers will accepts 2nd argument as sub store name.

computed: mapGetters({
  admin: 'isAdmin' // -> admin: 'user/isAdmin'
}, 'user'),

methods: mapActions({
  login: 'login' // -> login: 'user/login'
}, 'user')

@ktsn ktsn added the discussion label Oct 4, 2016
@Sarke
Copy link
Author

Sarke commented Oct 5, 2016

Thank you taking the time to discuss this with me, @ktsn. 😄

Namespace

Yes, I agree with what you said completely. The prefix string is a good idea.

Model feature

Actions and mutations can affect other modules and also other modules can dispatch or commit them.

Same with models, they can have methods that do certain actions that trigger other more broader changes.

The model interface is not intuitive for the actual behavior as it triggers some events rather than just assign values to properties.

Well as with any setter, it can do many things, not just act on itself.

We cannot use same name among state, getters, actions and mutations since they will assign on same object. This is inconsistent restriction with current store API.

I don't understand this point. You're not able to create an object that maps getters, setters, and methods to their equivalent store functions?

IMO, mapping a module as a model object would be bad idea

To be fair, and like I said earlier, I don't think anything here would change any current feature, it would simply add additional optional functionality. So it would be up to each individual to utilize those optional features if they so wish. But perhaps this would be better served as a vuex plugin/extension.

However, I like your "shortcut" idea as omitting prefix under some specific context. How would you feel about creating sub store instead of model? We can omit annoying prefix on a sub store.

I think this is a good compromise and would be more inline with the current implementation.

However, I think a change like this would be benefited with more discussion of collections with regard to the store. What I am really missing is getters on individual items in the store. E.g.:

cont user = store.state.users.items.get(5) // notice the plural of the module, and items is a Map()
user.isAdmin // can't use a getter here, vuex requires simple objects

So I want to get to a point where I can have my user.fullName or product.isOnSale, but since vuex doesn't handle deeper getters it would have to be something else.

Sorry for the tangent, but I think it's important to at least consider if collection handling will be a future feature when considering these changes. That said, I do like your substore idea.

@Sarke
Copy link
Author

Sarke commented Oct 5, 2016

To add to this, regarding your objections regarding the model feature. What I propose it inline with the mapX examples in the official documentation, for example:

    ...mapMutations([
      'increment' // map this.increment() to this.$store.commit('increment')
    ]),
    ...mapActions([
      'increment' // map this.increment() to this.$store.dispatch('increment')
    ]),

All perfectly valid and used as examples in the documentation. Now let's apply my user model example, which takes advantage of the subStore feature you suggested:

export default {
    computed: {
        ...mapState({
            id: state => state.id,
        }, 'user'),
        ...mapGetters([
            'isAdmin',
        ], 'user'),
    },
    methods: {
        ...mapActions([
            'login',
        ], 'user'),
        ...mapMutations([
            'name'
        ], 'user'),
    },
}

With that, I get the model I wanted (hey, components are just view-models).

user.id
user.isAdmin
user.login(credentials)
user.name = newName

All we would need is something to automatically map these based on the module.

@ktsn
Copy link
Member

ktsn commented Oct 5, 2016

I understand you want to just add the optional feature but don't want to change current API. I mean that we have different mental model between model and Vuex module. When we assign some values to model or call some methods on model, we expect they only affect the model itself. However, they can affect other modules in fact since they trigger global events under the hood. This inconsistency is because Vuex's concern is simplifying application data flow while model's concern is representing data structure.

For this reason, I think model should be independent on Vuex and Vuex modules have the model in their state. This would make sense for practical applications because their Vuex modules probably do not have one-to-one relationship with models. (e.g. Shopping cart app having a product list module and a cart module. Both they will have product model in their state)

In this approach, the collections problem would be solved since we can define our own getters in the in-state-model itself.

We cannot use same name among state, getters, actions and mutations since they will assign on same object. This is inconsistent restriction with current store API.

I don't understand this point. You're not able to create an object that maps getters, setters, and methods to their equivalent store functions?

For example, we cannot define update action and update mutation on same module when we want to use model because both of them will be assigned to same member user.update. Yes, this is an edge case and a small problem, also we can print an error message on this situation. However, I wonder why we should dare to include this restriction even though sub store interface does not have that.

Now let's apply my user model example, which takes advantage of the subStore feature you suggested:

The comparison is a bit unfair because the former one is the code for binding state, getters, actions and mutations to a component but latter is for getting/dispatching them. We can use sub store directly if we want to do that. The only difference between them is being model-like interface or store-like interface.

user.state.id
user.getters.isAdmin
user.dispatch('login', credentials)
user.commit('name', newName)

@Sarke
Copy link
Author

Sarke commented Oct 5, 2016

For this reason, I think model should be independent on Vuex and Vuex modules have the model in their state. This would make sense for practical applications because their Vuex modules probably do not have one-to-one relationship with models. (e.g. Shopping cart app having a product list module and a cart module. Both they will have product model in their state)

In this approach, the collections problem would be solved since we can define our own getters in the in-state-model itself.

Interesting. I did not think Vuex could handle anything complex in the state. This would work for me for sure, but how would it be accomplished?

If we can have in-state models with getters in collections, that would be mint. The substore you suggested likewise. With those two, we can drop my suggestion for the module-model.

@ktsn
Copy link
Member

ktsn commented Oct 6, 2016

Plain JavaScript class is acceptable in Vuex state. https://jsfiddle.net/uzt0eqm1/

@Sarke
Copy link
Author

Sarke commented Oct 7, 2016

That's great, so yeah, scrap everything I said about the module-model, I'll use this instead!

Substore is still a great idea though.


EDIT: the best part about this is I can not only have my getters and setters, but I can create relations to the other resources like I am used to! 😄 Thanks again!

@ktsn
Copy link
Member

ktsn commented Oct 8, 2016

I'm glad to reach consensus. Thank you for your proposal and discussion!
Sub store idea was never appeared without your initial idea 😄

Closing this issue now and I'll open some new issues for the ideas came up in this discussion.

@ktsn ktsn closed this as completed Oct 8, 2016
This was referenced Oct 9, 2016
@ktsn
Copy link
Member

ktsn commented Oct 9, 2016

I've opened new issues #380, #381.

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

2 participants