davestewart / vuex-pathify

Vue / Vuex plugin providing a unified path syntax to Vuex stores
https://davestewart.github.io/vuex-pathify
MIT License
1.37k stars 57 forks source link

mapMutations equivalent #102

Closed ajenkins-cargometrics closed 4 years ago

ajenkins-cargometrics commented 4 years ago

I've found pathify to be very useful. One feature it seems to be missing though is a direct equivalent of the vuex mapMutations function. I think the intention was that most use cases would be covered by sync, however this doesn't cover use cases where there isn't one-to-one correspondence between state properties and mutators. For example my state might have a users list property, and then addUser and deleteUser mutators. None of the existing component helper functions seem to cover mapping these mutators.

My current options are:

  1. Use vuex's mapMutations for these. This works fine, but just seems a bit messy since I can't just consistently switch to using pathify in my components.
  2. Use this.$store.set('addUser!', user) in my component code. Works, but isn't as nice as mapping to methods. Of course I could manually write method wrappers that do this, but...
  3. Make action wrappers around mutations, so that I can use the call component helper. Works, but is extra boilerplate, and now my mutators are asynchronous just to satisfy pathify, when I didn't actually want them to be.

What I'd like:

Two options I can think of off the top of my head:

  1. Add a set component helper that works similarly to the others, but understands the ! syntax, which supports code like:

    methods: {
    ...set(['addUser!', 'deleteUser!']),
    
    handleAddUser (newUser) {
        this.addUser(newUser)
    }
    }
  2. Make the call component helper handle mutations. It could use a priority ordering that prefers actions, but falls back to a mutation.

davestewart commented 4 years ago

Hey,

Thanks for the suggestions.

I actually did have a set() helper a while back but I took it out. I think because it just wasn't particularly intuitive, but I can't remember now.

I get the use case, though I'm not actually devoting any time to OSS at the moment because of another project I'm on.

See how you go, and if you're in the same boat in a couple of months, maybe I'll take a look.

I'll leave this ticket open!

Cheers, Dave

ajenkins-cargometrics commented 4 years ago

Thanks. I'd be fine with submitting a PR too when I get some free time. Say I created a set() component helper which behaved similarly to sync, but where sync creates getters and setters, set would create only the setters, as methods. Additionally, the set helper would understand the ! syntax just like the set store accessor.

So for example:

methods: {
   addUser: set('users/addUser!')
}

generates

methods: {
    addUser (payload) {
        this.$store.set('users/addUser!', payload)
    }
}
methods: {
    status: set('products/status')
}

generates

methods: {
    status (payload) {
        this.$store.set('products/status', payload)
    }
}

Additionally, the map, array, and wildcard syntax can be supported just as for sync.

methods: {
    ...set('products', ['order', 'key', 'addProduct!'])
}

generates

methods: {
    setOrder (payload) {
        this.$store.set('products/order', payload)
    },
    setKey (payload) {
        this.$store.set('products/key', payload)
    },
    addProduct (payload) {
        this.$store.set('products/addProduct!', payload)
    }
}

Note that in the expanded versions I showed using the set store accessor instead of directly calling $store.commit, so that it will prioritize calling actions over mutations, just like sync() or the set() store accessor would.

One wrinkle is deciding what the generated method names should be when using array or wildcard syntax. Since there can be different mapping rules for actions and mutations, if the method names were the same as the mapped-to mutation or action, this could lead to surprising results. The standard mapping maps mutations to SET_FOO and actions to setFoo, so it seems like it would be kind of surprising if you could end up with a SET_FOO or setFoo method on your component depending on whether or not the setter for foo was an action. Some options I can think of are:

  1. Use camel-case method names
  2. Use the mapping for actions to generate method names
  3. Limit the set component helper to map syntax for multi-property access, to make users explicitly specify the names.

Does the above sound like something you might accept?

davestewart commented 4 years ago

Hey hey,

So I presume you can see the issue here... set() potentially becomes add, delete etc. depending on your mutation handlers, which feels a bit icky.

Maybe modifying the commit alias would be better here:

https://davestewart.github.io/vuex-pathify/#/api/properties?id=vuex-aliases

So the code would be something like:

methods: {
  ...commit('users', [
    'addUser',
    'updateUser',
    'deleteUser',
  ])
}

Two thoughts:

I'd probably advise against submitting a PR because I just don't have the time to look at them right now (and haven't for about a year). I'm a bad OSS author!

Also, this would be low down my priority list. The next two things I would like to do would be:

In the meantime I'd recommend writing a helper which would do this for you. It's probably about 15 - 20 lines of code, to handle path, object or array cases. Look in the source code and copy if you like:

https://github.com/davestewart/vuex-pathify/blob/f657ac127abdec286666679b718cec5c75853d23/src/services/paths.js#L20

If you are worried about imports, just re-export the entirety of pathify from your own pathify config, and alias it in your webpack setup so you could do something like this:

import { get, symc, blah } from '@/pathify`

Have fun!

ajenkins-cargometrics commented 4 years ago

Thanks for the response. For my own current use cases, just using Vuex mapMutations works, so if you don't think you'd accept a PR I'll just continue using mapMutations.

addUser was maybe a bad made up example on my part, because its name does seem to imply it would involve some sort of server call, in which case you're right that it would be an action, not a mutation. In my actual code though I have a number of mutations that just manipulate local data structures in the store, so there's no reason for them to be actions.

Regarding Vuex removing mutations in favor of actions, is that something the Vuex maintainers have said?

davestewart commented 4 years ago

OK, cool.

Regarding Vuex removing mutations in favor of actions, is that something the Vuex maintainers have said?

Yeah, it's been public knowledge for a while.

I think it makes a lot more sense and will reduce the confusion between actions and mutations.

I don't know what impact it will have on how we Vuex, because actions are async but commits are sync, but hopefully they will come up with something that's not awful.

Though saying that, Vuex is awful at times. Let's hope, as least-awful as can be hoped!

davestewart commented 4 years ago

As you're happy with mapMutations I'll close the issue for now :)

ajenkins-cargometrics commented 4 years ago

No problem. Thanks for being so responsive, and for creating this useful library.