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

Add subproperties on objects #14

Closed baikunz closed 6 years ago

baikunz commented 6 years ago

Is there a way to add subproperties to an existing object in the store that I don't know of ?

// Store.js
const state = {
    element: {},
    loading: false,
};

const mutations = make.mutations(state);

export default {
    state,
    mutations,
}
// ElementInput.js
export default {
    name: 'ElementInput',
    computed: sync(['element@title']),
}

In this case the SET_ELEMENT mutation created by vuex-pathify is called but does not mutate my element state to add the title property. It stays an empty object.

I have to manually create the mutation method myself to make it work:

// Store.js
// Some code...
SET_ELEMENT(state, payload) {
    payload instanceof Payload
        ? state.element = { ..._set(state.element, payload.path, payload.value) }
        : state.element = payload || {}
    ;
},
// Some code

Is it the expected behaviour ?

davestewart commented 6 years ago

Looks like you're using namespaced paths but your module isn't namespaced. Typo, or actual code?

baikunz commented 6 years ago

Yes typo sorry, I removed the namespaced: true at the top of my store just for the submission. Will edit to make it more clear ;-)

davestewart commented 6 years ago

I'm just checking it out now in a sandbox, and it looks like the property is NOT created.

It seems like a reasonable use case - if this is an omission, I'm surprised I didn't test for this when building the demos!

I can take a look at this if you like, at the very least to see if just the way I've coded it that it isn't setting something properly.

Maybe I can put the behaviour behind a flag.

baikunz commented 6 years ago

Yes this is not the first time I encounter this issue. I often initialize my state objects with an empty literal object, and then add properties through mutations if needed. I had to make my custom mutation to make it work, I did not go thought the Payload.update method, but I'm pretty sure this is where something's missing ;-)

Thanks !

davestewart commented 6 years ago

OK, leave it with me. I'll try and get to this tonight if possible!

davestewart commented 6 years ago

So I've been doing a bit of experimenting, and:

  1. I think I know why I didn't implement this
  2. adding properties dynamically seems to bypass Vue's reactivity system

Issue 1

The issue with setting properties dynamically using paths, is that it would be really easy to nuke existing properties. Say foo is 1, then you set foo.bar = 2; that converts foo into an Object and sets a key bar with value 2.

Pathify's sub-properties functionality was designed primarily to provide access to grouped properties for example sort@key or car@body.color rather than adding or removing properties. I worry that adding this functionality (even though it seems quite useful) could create problems.

Issue 2

I've updated the setValue() function and I can successfully set the store value to any depth, simply by providing a path.

However, if the state is not predefined, any component getters (even manually created) do not fire when the state is updated.

I'm concluding that this is because Vuex's state is actually a Vue instance, which has to have its data provided so that it can be made reactive, see the reference here.


Perhaps you can elaborate on your use case.

Is there a reason that title should be dynamic?

Maybe there's a workaround such as using Vue.set() ?

davestewart commented 6 years ago

Was thinking about this a bit more.

Turns out you can use Vue.set() to "upgrade" the state on the fly.

greeting: sync('value@a.b.c'),
Vue.set(store.state.value, 'a', {b: {c: 'Hello there'}})

The UI then magically starts working.

So it's possible I could upgrade setValue() to use Vue.set() but I think to do this, the current Vue instance would need to be injected somehow, as I think using import Vue from 'vue' inside the package introduces tight coupling (this is why plugins use Vue.use())

So again, if you can let me know your use case.

Either that, or there might be a way to upgrade Payload leaving the main lib untouched.

Food for thought anyway....

davestewart commented 6 years ago

OK, here's a patch to upgrade Payload:

import Vue from 'vue'
import { Payload } from 'vuex-pathify'

Payload.prototype.update = function (target) {
  const keys = this.path.split('.')
  keys.reduce((target, key, index)  => {
    if (!(key in target)) {
      Vue.set(target, key, {})
    }
    if (index === keys.length - 1) {
      target[key] = this.value
    }
    return target[key]
  }, target)
}

I've done some light testing and it seems pretty successful.

computed: {
  a: sync('value@foo.bar.a'),
  b: sync('value@foo.bar.b'),
  c: sync('value@foo.bar.c'),
}
store.set('value@foo.bar.x'),

Have a go with that and see how far it gets you, then maybe we can look at integrating it.

Do let me know some use cases again!

baikunz commented 6 years ago

Hi thanks for the detailed answers, I'm sorry I had no time to get back to you. I'll try to reply ASAP ;-)

davestewart commented 6 years ago

Note that this approach only upgrades sub-properties.

Not sure if there is any value in allowing the same functionality on main store properties...

baikunz commented 6 years ago

Okay, let's try to recap! Sorry for the late reply, been quite busy.

The issue with setting properties dynamically using paths, is that it would be really easy to nuke existing properties. Say foo is 1, then you set foo.bar = 2; that converts foo into an Object and sets a key bar with value 2.

Well it's a valid point, but we could imagine testing that foo is at least an object before updating it's subproperty and maybe throwing an explicit error to make it clear to the end user.

So it's possible I could upgrade setValue() to use Vue.set() but I think to do this, the current Vue instance would need to be injected somehow, as I think using import Vue from 'vue' inside the package introduces tight coupling (this is why plugins use Vue.use())

Indeed you can use Vue.set() to add reactive properties to an existing object. But you could also create an entirely now object like explained at the end of this paragraph.

This is what I did on the code I provided above using lodash:

state.element = { ..._.set(state.element, payload.path, payload.value) };

Not the prettiest but it keeps reactivity on newly added properties without the need to import Vue. Of course you could use your own _.set method to avoid importing lodash/set too.

Note that this approach only upgrades sub-properties. Not sure if there is any value in allowing the same functionality on main store properties...

Do let me know some use cases again!

I can see why you'd rather not add sub-properties on main store properties. In a perfect situation you would be able to create a initial state with all properties / sub-properties initialized, even if they are null or empty objects or whatever. Something like that:

const state = {
    prop1: {
        sub1: {
            sub11: null,
            sub12: '',
        },
        sub2: false,
    },
    prop3: null,
}

In my case, where I'm often prototyping I might have to quickly add properties to an object in the state, and I'd rather not have to update my initial state everytime I'm adding a new property. That and the fact that I'd rather have an omitted property on my state element than a null/''/{} valued property if there is no actual value. In my case, it's mostly used for data coming from the database that I need to share between components. Most of the time for pure UI, my state is initialized like described above. I know this is not a perfectly valid use case nor a good practice but it works in my workflow. Anyway, in my case i'd initialize my state like that:

const state = {
    prop1: {},
    prop3: null,
}

We could even log a warning if we are adding a prop missing from the initialized state to avoid unwanted side effects.

Let's hope I did not forgot anything !

davestewart commented 6 years ago

Good reply, thanks!

Nice trick with Object.assign(). I can look to use that either in Payload or in the generated mutation function and hopefully that will get us there. Hopefully will get a bit of time to work on that tonight.

Fingers crossed!

davestewart commented 6 years ago

OK, I've got this working!

There might be a slight change to the API; if so it will need to be a minor (breaking change) release. I'll release a couple of other features at the same time, so probably won't happen for a week or so.

The next thing to work out is how to enable it. I'm thinking changing the options from:

deep: false  // disable access to sub-properties
deep: true   // enable access to sub-properties

to:

deep: 0      // disable access to sub-properties
deep: 1      // enable access to existing sub-properties
deep: 2      // enable access to existing properties plus creation of new sub-properties

Thoughts?

But I think this is a nice feature, certainly for sub-properties, I think I like it!

baikunz commented 6 years ago

Nice !

Regarding the options, your exemple seems good, and won't cause any breaking change if proper tests are made (allowing false/0 and true/1 to preserve compatibility). The values are a little less self-explanatory but that could be enough ;-) I was thinking of adding another option which would enable a warning if new properties were created but I don't now actually :P

davestewart commented 6 years ago

So the breaking change to accomplish the reactivity is that payload.update() no longer sets values; you need to do:

state.<key> = payload.update(state.<key>)

This is only when setting values manually. Apart from that, everything is the same. And yes 0 == false and 1 == true!

Also, options.deep can now be set on the fly. I think this is a good change... feel free to comment.

I'm bundling up a bunch of other new features into a 1.1 release.

Hoping to get tests written as well, but that might end up being a 1.2 release :)

baikunz commented 6 years ago

Got it ! Just be careful of the semver, if you break compatibility you should bump a major version. It looks like payload.update is being changed from the previous releases so you should definitely bump your version to 2.x in case someone is relying on this API.

Thank you for your work mate, it made my vuex workflow much easier :D

davestewart commented 6 years ago

Dammit, I forgot / hate how breaking changes bump the major number.

They really need 4 components: generation, major, minor, patch where generation denotes the level of re-write (think Windows XP, 7, 10)

I might just risk this one 😝

davestewart commented 6 years ago

Available on develop now :)

davestewart commented 6 years ago

You will need to update options.deep either using the special import configuration, or if you won't be adding properties until a component runs, you can do it anywhere in main.js

import Pathify from 'vuex-pathify'

// before running any code
Pathify.options.deep = 2

// rest of code...

You can even do it in the component / action you'll be working in if you need to, and turn it off afterwards!

davestewart commented 6 years ago

By pure luck, it turns out this is NOT a breaking change!

It's only a breaking change if you were expecting to create new properties anyway, which was never supported.

If the properties exist, this works exactly the same as before, as the original object does not need to be re-assigned!

Payload.update(state.foo) // doesn't re-assign, works for existing
state.foo = Payload.update(state.foo) // does re-assign, works for new

😁

baikunz commented 6 years ago

Perfect ;-) gonna test it ASAP thanks !