vuejs / vuex

🗃️ Centralized State Management for Vue.js.
https://vuex.vuejs.org
MIT License
28.41k stars 9.57k forks source link

v4 reactive getters behaviour appears to have changed for nested methods? #1890

Open songololo opened 3 years ago

songololo commented 3 years ago

What problem does this feature solve?

I used to be able to apply a reactive getter to a nested method inside an object instance stored in my v3 vuex state, but this doesn't appear to work anymore in v4.

In my case it was a mapbox instance which was created and then stored inside the vuex v3 store's state. I could then share this instance amongst various components and could also watch the instance for changes via reactive getters.

For example:

state: {
  mapInstance: null, // <- gets assigned with a mapbox instance
  // mapInstance contains a method called "getZoom()"
},
getters: {
  zoom: (state) => (state.mapInstance ? state.mapInstance.getZoom() : null),
}

The same approach no longer works in vuex v4.

What does the proposed API look like?

I'm assuming something has changed in the internals of vuex v3 vs. v4: is there a way to deliberately expose certain nested properties or methods to reactive getters in vuex v4? Or else to enable the previous functionality?

kiaking commented 3 years ago

@songololo This might be related to #1889. Could you test with Vuex 4.0.0-RC.2?

songololo commented 3 years ago

I am still encountering the same issue on 4.0.0-rc.2

kiaking commented 3 years ago

Oh, OK. Hmmm... let me look into it.

kiaking commented 3 years ago

@songololo OK, I can't reproduce the issue. I've tried first defining state.mapInstance as null, then, set new MapInstance with a mutation. Then I correctly got the getZoom method. So the following code is working.

<template>
  <div id="app">
    <button @click="setInstance">SET INSTANCE</button>
    <p>{{ zoom }}</p>
  </div>
</template>

<script>
class MapInstance {
  getZoom() {
    return 'Zoom!'
  }
}

export default {
  computed: {
    zoom () {
      return this.$store.getters.zoom
    }
  },
  methods: {
    setInstance () {
      this.$store.commit('setMapInstance', new MapInstance())
    }
  }
}
</script>

When you click SET INSTANCE button, then the 'Zoom!' string is displayed on the screen. Is this what you're trying to do? Please provide reproduction code that I can look into.

songololo commented 3 years ago

Thanks. I added a reproduction here: https://github.com/songololo/test-vuex

I am trying to use a Mapbox instance via the store, which is something I used to be able to do in Vue v2 with the old vuex.

kiaking commented 3 years ago

@songololo Thank you for the code! OK, so I've run the code, and got this.

Screen Shot 2020-12-01 at 9 45 35

getZoom is returning 0, so... it's working?

I wondered maybe you wanted to getZoom to return a new value each time an user scroll the map, in that case, it's not working. However, I've tested with plain object that mutates it self, like this.

{
  state: {
    obj: {
      valueA: 1,
      edit () {
        this.valueA++
      }
    }
  }
}

Then it worked. The obj.valueA will get updated every time I call obj.edit() method in component.

Could you describe your issue a little more so I understand what I'm trying to look at. Also, if it would be very helpful if you could also provide Vuex 3 version of the working code as well 🙏

songololo commented 3 years ago

Thanks for looking: The zoom level is meant to track the actual zoom state of the map, which starts around 3 then changes reactively when scrolling in and out (zooming in and out) over the map.

I've created a repo using Vue 2 with Vuex 3 over here: https://github.com/songololo/test-vuex-3, which shows the reactive behaviour in action.

kiaking commented 3 years ago

@songololo Thanks for the reproduction. Now I see what is going on. Indeed Vuex 3 is updating the state. I did few tests.

If we assign a new class, and then update the state, it will be reactively updated. Like this.

// store.js

import { createStore } from 'vuex'

class Obj {
  constructor() {
    this.valueA = 1
  }

  getA() {
    return this.valueA
  }

  update() {
    this.valueA++
  }
}

export default createStore({
  state: {
    obj: null
  },

  mutations: {
    setObj(state) {
      state.obj = new Obj()
    }
  },

  actions: {
    initObj({ commit }) {
      commit('setObj')
    }
  },

  getters: {
    getObjA: (state) => {
      return state.obj ? state.obj.getA() : 'nope'
    }
  }
})

This is essentially the very simple version of mapInstance implementation. Then, if we use this in Vue Component like this, it works.

<script>
import { computed, onMounted } from 'vue'
import { useStore } from 'vuex'

export default {
  name: 'App',

  setup() {
    const store = useStore()

    onMounted(async () => {
      await store.dispatch('initObj')

      // Update state value on every scroll.
      window.addEventListener('scroll', (s) => {
        store.state.obj.update()
      })
    })

    const getA = computed(() => {
      return store.state.obj ? store.state.obj.getA() : 'nope'
    })

    return {
      mapDiv,
      getA
    }
  }
}
</script>

So, "nested methods" is working as same as Vuex 3 at this point. However, I still don't know why mapInstance is behaving differently.

This issue also happens when using plain reactive from Vue 3, so technically, this is not related to Vuex, but rather difference between Vue 2 vs. Vue 3 reactivity system.

We need a more minimal reproduction code for Map instance behaviours to dig in further,


Request to you

Q. Would it be possible for you to dig into Map class on mapbox-gl to see how getZoom is implemented? And what exact property is being changed during the map zooming event?


What happens from here on

Since it's stated here at docs that:

The data you store in Vuex follows the same rules as the data in a Vue instance, ie the state object must be plain.

It is best to not store this kind of class instance to state in the first place, I'll recommend you to modify your code to change how you handle the Map instance. Because of this, I would suppose this issue will not be handled in the near future. Also, if we were to address this issue, the change has to be made in Vue side, not in Vuex.

However, I'm not saying this should never be addressed. At the moment, I don't know. If we can determine what is the root cause of the issue, we might find a way out.

If anyone can help us with debugging the Map instance behaviours, it would be really nice 🙌

songololo commented 3 years ago

Thanks @kiaking

For now I've changed approach to work with the new behaviour: I've attached event watchers to the Map object and these, in turn, update the state when the map is moved (and I can then plumb the reactive getters to those instead).

Regarding mapbox-gl and the getZoom method: The Map class extends a Camera class which contains the getZoom method, which looks like this:

    /**
     * Returns the map's current zoom level.
     *
     * @memberof Map#
     * @returns The map's current zoom level.
     * @example
     * map.getZoom();
     */
    getZoom(): number { return this.transform.zoom; }

So, the action is happening at two levels deep: Map -> transform -> zoom

kiaking commented 3 years ago

Great to hear you found a way out!

So, the action is happening at two levels deep: Map -> transform -> zoom

Yeah... though it's strange. I don't see why nested function wouldn't work. Maybe we can come back and check if something would change in Vue 3 👀

unadlib commented 3 years ago
  it('getters', () => {
    const fn = jest.fn()
    const store = new Vuex.Store({
      state: {
        a: 0
      },
      getters: {
        state: state => {
          fn()
          return state.a > 0 ? 'hasAny' : 'none'
        }
      },
      mutations: {
        [TEST] (state, n) {
          state.a += n
        }
      },
      actions: {
        check ({ getters }, value) {
          // check for exposing getters into actions
          expect(getters.state).toBe(value)
        }
      }
    })
    expect(store.getters.state).toBe('none')
    store.dispatch('check', 'none')

    store.commit(TEST, 1)

    expect(store.getters.state).toBe('hasAny')
    expect(store.getters.state).toBe('hasAny')
    expect(store.getters.state).toBe('hasAny')

    expect(fn.mock.calls.length).toBe(5)
  })

The above issue occurred from 4.0.0 rc.2 to 4.0.0, fn.mock.calls.length should be expected to be 2.

kiaking commented 3 years ago

@unadlib That's expected at the moment. Getters are not cached. We need Vue 3.1 to fix it. See https://github.com/vuejs/vuex/pull/1883 for more details.

It's not related to this issue I think though.