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

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

Open
songololo opened this issue Nov 26, 2020 · 12 comments
Open
Labels

Comments

@songololo
Copy link

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
Copy link
Member

kiaking commented Nov 30, 2020

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

@kiaking kiaking added 4.x need more info Further information is requested labels Nov 30, 2020
@songololo
Copy link
Author

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

@kiaking
Copy link
Member

kiaking commented Nov 30, 2020

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

@kiaking
Copy link
Member

kiaking commented Nov 30, 2020

@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.

@kiaking kiaking added need repro Reproduction code is required and removed need more info Further information is requested labels Nov 30, 2020
@songololo
Copy link
Author

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
Copy link
Member

kiaking commented Dec 1, 2020

@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
Copy link
Author

songololo commented Dec 1, 2020

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
Copy link
Member

kiaking commented Dec 2, 2020

@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 🙌

@kiaking kiaking removed the need repro Reproduction code is required label Dec 2, 2020
@songololo
Copy link
Author

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
Copy link
Member

kiaking commented Jan 4, 2021

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
Copy link

unadlib commented Feb 5, 2021

  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
Copy link
Member

kiaking commented Feb 5, 2021

@unadlib That's expected at the moment. Getters are not cached. We need Vue 3.1 to fix it. See #1883 for more details.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants