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

Nested mutations #828

Closed
jdcrecur opened this issue Jun 9, 2017 · 7 comments
Closed

Nested mutations #828

jdcrecur opened this issue Jun 9, 2017 · 7 comments

Comments

@jdcrecur
Copy link

jdcrecur commented Jun 9, 2017

What problem does this feature solve?

Solves the duplication of mutation function names.

  mutations: {
    phaseXAdd (state) {
      state.currentBoard.phases.x.push({
        name: 'New Phase',
        description: 'This is a new phase',
        span: 1
      })
    },
    phaseXRemove (state, index) {
      state.currentBoard.phases.x.splice(index, 1)
    },

In the above, the string "phase" and "X" have to be written down for each function. This both increases the code base size and increases the time for development, this also is more prone to typo's.

What does the proposed API look like?

 mutations: {
    phase: {
      x: {
        add (state) {
          state.currentBoard.phases.x.push({
            name: 'New Phase',
            description: 'This is a new phase',
            span: 1
          })
        },
        remove () {
           state.currentBoard.phases.x.splice(index, 1)
      },

Now to call the "phase x add" the syntax is the std . spearated syntax:
this.$store.commit('phase.x.add')

This now allows grouping within modules where each nesting within a single module can directly affect its own state. The code qty is reduced and easier to navigate and easier to call from a commit.

To call nested mutations within modules is also clear and simple:
this.$store.commit('board/phase.x.add')

@Austio
Copy link
Contributor

Austio commented Jun 9, 2017

Have you considered using modules in store for this, I think you could accomplish what you are describing with it

https://vuex.vuejs.org/en/modules.html

@jdcrecur
Copy link
Author

jdcrecur commented Jun 9, 2017

From the vuex docs:
"Similarly, inside module actions, context.state will expose the local state, and root state will be exposed as context.rootState"

Within a child module mutation function, the said function cannot directly manipulate the root.state unless accessed via by dispatching an action which is way overkill for what nesting mutations could do. The modules ability to hold their own state is great but not always needed.

By nesting you are simply grouping one modules mutations to reduce the overall code written. Name spacing each module and then using the / operator is great but they are two different things in my mind.

If the a child module's mutation could gain access to the rootState of the parent stroe this would also be an improvement. But regardless, nesting a store's mutations using the . syntax would still result in a small code base for the app. The app i am building with Vue is basically a GUI onto a mongo document with the Vuex store sending store updates with socket.io (it is lightening fast and so far Vue + Vuex is working like a dream).

Where it would be great to get to is syntax like this:
this.$store.commit('user/board/phase.x.add')

@ktsn
Copy link
Member

ktsn commented Jun 10, 2017

I'm not sure this is a good idea because we wouldn't able to extend the mutation syntax if we introduce this. (e.g. We cannot add #770 if we allow nested mutation declaration)
I'm also considering the syntax makes static analysis harder like type checking by TypeScript and Flow since it dynamically creates mutation names.

I think this should be implemented as a mutation enhancer that is separated with Vuex.

For example we can introduce nestedMutations enhancer:

mutations: nestedMutations({
  phase: {
    x: {
      add (state) {
        //...
      },
      remove (state) {
        //...
      }
    }
  }
})

The above code is equivalent with:

mutations: {
  'phase.x.add' (state) {
    // ...
  },
  'phase.x.remove' (state) {
    // ...
  }
}

@ktsn ktsn added the proposal label Jun 10, 2017
@LinusBorg
Copy link
Member

@ktsn I agree with you. This enhancer could be published as a separate utility though, right? It woulnd't have to be done in core.

So I vote to close this request.

@jdcrecur
Copy link
Author

jdcrecur commented Jul 2, 2017

OK, I wrote the enhancer then and published it. I also have it working on a project I am working on. It was a great idea to abstract this from @ktsn :

NPM:
https://www.npmjs.com/package/vuex-nested-mutations

GIT:
https://github.com/jdcrecur/vuex-nested-mutations

@LinusBorg
Copy link
Member

Great!

@jdcrecur
Copy link
Author

jdcrecur commented Jan 2, 2018

I know this is closed, but i was hoping to maybe either re-open it or get some clearer insight as to why this is not possible.

The vuex-nested-mutations package i wrote enables nested mutations, and cleans up code nicely and thus reduces the qty of code sent out to production. But... the IDE cannot autocomplete against the mutation object and the object itself is flattened by the vuexNestedMutations function. The inability to autocomplete results in typos and generally slows the developer down..

If nested mutations were written into the core:
1 - The ide could auto-complete against the mutation object directly
2 - Changes to the mutation object would through up errors straight away. Currently passing a string does not get the ide to throw up errors, as far as the ide is concerned a valid string param is passed to the commit function.
3 - The trend to have all mutation types as constants, like with redux.. this offers autocompletion in the ide and to a degree prevents some collision, but not required if the mutations are a simple nested object as the nested'ness is their localised namespace.
4 - Readability is vastly increased along with less potential collisions. In a giant file of constants is not unimaginable that the values of the constants may accidentally be the same thus a collision. With a nested object is is very difficult for this to happen as long as the highest level keys are different.

If you don't need that annoying mutation-types file that is one less file to maintain... i added a snapshot of a new small app i just built using the suggested constants method... nobody can agree that this is easy to read and thus quick and easy to maintain, this is after a couple of hours and it is already just a long terrible list only a computer is good are parsing:
https://ibb.co/hN2VHG

This is the same code but nested: https://ibb.co/jQjcxG so much more easier to understand at a mere glance!

https://ibb.co/f1JZBb
If you could then call the mutation functions directly that is

@ktsn You mentioned that if this was added to the core it would reduce the ability to extend the mutations sytanx in the future, but if the core reduced a mutation nested object to flattened array of camelCase keys sure everything remains the same. As will any testing.. ? Thus I would argue the benifit of increased dev time and reduction of typos was worth it. Plus developers are lazy and don't want to type if they don't have to.. ;)

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

4 participants