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

getters result doesn't get cached #2102

Closed
BoHuny opened this issue Dec 30, 2021 · 18 comments
Closed

getters result doesn't get cached #2102

BoHuny opened this issue Dec 30, 2021 · 18 comments

Comments

@BoHuny
Copy link

BoHuny commented Dec 30, 2021

Version

4.0.2

Reproduction link

codesandbox.io

Steps to reproduce

  • Instanciate a Vuex store
  • Call a getter multiple times without change in the parameters
  • The getter gets called each time as the result isn't cached

What is expected?

The getter function should be computed once only (as in this Vue 2 minimal reproduction: https://codesandbox.io/s/interesting-cerf-22fu3?file=/src/main.js)

What is actually happening?

The getter function is called each time


Vue version: 3.2.26 (doesn't work on 3.1.5/3.0.11 neither)

@kyrsquir
Copy link

kyrsquir commented Jan 4, 2022

Glad we are not the only ones facing this, we started to think that no one else is using getters with Vuex 4, that somehow the store usage paradigm has shifted and that we will need to rewrite most of our store code to keep up with it.

Our use case:

We are migrating our app from vue^2.6.14 + vuex^3.6.2 to vue^3.2.26 + vuex^4.02.
The app is making heavy use of vuex getters. We have literally thousands of lines of our app code in Vuex getters.
Many of those getters are executing CPU intensive operations.
While testing performance of the vue3 app version we found that vuex getters execute multiple times, despite their reactive dependencies are not changing (!) - this is not the case in vue2.6 where getters results get cached.
This behavior change causes a significant performance regression: the app is slower in tests. Such a performance regression is sufficient to hold back the update from the product’s performance perspective.

Any advice on what can we do to switch to Vue3 yet keep the performance as high as in Vue2.6?

This issue is also referenced in the vuex next docs:

As of Vue 3.0, the getter’s result is not cached as the computed property does. This is a known issue that requires Vue 3.1 to be released.

Both Vue 3.1 and 3.2 were released since then, but that note was never updated.

@BoHuny
Copy link
Author

BoHuny commented Jan 4, 2022

A solution we found was to replace the direct use of getters with a mapGetters to generate cached computed properties: https://next.vuex.vuejs.org/guide/getters.html#the-mapgetters-helper
(This is also considered good practice so it could be the occasion to refactor the sources, I guess)

@igal1c0de4n
Copy link

@BoHuny the solution you mention is applicable for getters which are accessed from components, correct?

In our case, there's an intricate getters interdependency within other vuex getters. So getter A depends on getter B which depends on getter C, etc.

For this type of usage, cached computed properties add little value. That's because when getter A recomputes it forces many other computationally intensive getters to recompute. This is a clear regression from vue2.6(+vuex) where they are cached.

@BoHuny
Copy link
Author

BoHuny commented Jan 5, 2022

@igal1c0de4n You're right, I didn't think about that particular use case
In that case if there's no other workaround it's clearly a regression from vuex3 as you mentioned

@phil294
Copy link

phil294 commented Jan 13, 2022

This was attempted to be fixed in #1883 (not yet released) but as you can see in that thread, this fix introduced another bug, breaking reactivity (even worse than before)

The official recommendation now is to use Pinia instead of Vuex. This only applies to new projects though, old codebases are forcefully stuck on Vue 2 until this is fixed. Tbh I think Even You should likely be pinged about this, as Vue 3 has been out for 16 months already, but honestly at this point I'm too tired to keep insisting on this.

@kyrsquir
Copy link

@yyx990803 Could you please take a look at this?

@8bitDesigner
Copy link

Yep, mapGetters or referencing the store property via computed in your component is a good work around. I've been using this for the last few months without issue. For example:

<script>
export default defineComponent({
  getters: {
    myStoreProperty () {
      return this.$store.getters.myStoreProperty
    }
  }
})
</script>

(or computed(store.getters.myStoreProperty) if you're using the composition API)

@leo-buneev
Copy link

leo-buneev commented Feb 7, 2022

Here's another way to workaround without refactoring - moving getters to components completely, works for any case, including one from @igal1c0de4n

before:

// store.js
export default {
  getters: {
    computedValue(state) {
      return complexComputation(state)
    }
  }
}

after:

// store.js
import gettersCache from './gettersCache'
export default {
  getters: {
    computedValue() {
      return gettersCache.computedValue()
    }
  }
}

// gettersCache.js 
import { createApp } from 'vue'
import GettersCachingComponent from './GettersCachingComponent.vue'

const app = createApp(GettersCachingComponent)
export default app.mount('#gettersCachingApp')

// GettersCachingComponent.vue
<template>
  <div />
</template>

<script>
import $store from '@/store'

export default {
  computed: {
    computedValue() {
      const state = $store.state
      return complexComputation(state)
    }
  }
}

@bill-kasa
Copy link

bill-kasa commented Feb 14, 2022

So, this is still an issue? Currently migrating from Vue 2 to Vue 3 and this is concerning.

@igal1c0de4n
Copy link

igal1c0de4n commented Feb 15, 2022

@leo-buneev the approach is interesting.
I'm concerned about computedValue getting executed excessively.
Sure, it's small however multiply by hundreds of getters accessed in hundreds of component lines...

Also, it requires refactoring >1000 lines of code in my project.
I'm concerned that after such refactoring, I'll find out the performance is not better or even worse than v2.6.

Lastly, it could get solved "by design" in a new Vue/Vuex release. If this happens, I'll need to reverse-refactor everything.

For now I'll be staying on 2.6 until the plan to address this issue become clearer

@fgarit-te
Copy link

@bill-kasa still an issue as of vuex 4.0.2. Pretty critical issue, I might not have migrated my app to Vue 3 had I known about this. I also can't afford another migration to Pinia right now :/

@bill-kasa
Copy link

@bill-kasa still an issue as of vuex 4.0.2. Pretty critical issue, I might not have migrated my app to Vue 3 had I known about this. I also can't afford another migration to Pinia right now :/

Agreed. This seems like something that should be front-and-center on the Vue 3 Migration guide on the website.

@csicky
Copy link

csicky commented Mar 11, 2022

Is somebody working on fixing this? It seems Vue 2 is not upgradable after all, only for small apps and demos.
I too have many getters depending on each other and I have also console logs in many of them. My surprise was so big when at first start of the project, after a very big work on migrating it, the console is flooded and settles only after long time.

@fgarit-te
Copy link

fgarit-te commented Mar 11, 2022

Not entirely relevant, but to counter this problem, my team has started migrating to Pinia... Though Pinia is a clear upgrade from Vuex and the migration is pretty smooth (we're migrating modules one by one), fixing the tests is what takes us the most time. And we would have preferred doing this migration on our own terms instead of somewhat forced.

(For more context, Pinia is going to be rebranded into Vuex 5, according to Evan You, so it's a safe bet)

@csicky
Copy link

csicky commented Mar 11, 2022

Thank you for your suggestion, I will check it out.

@LifeIsStrange
Copy link

Has anyone investigated wether Pinia cache getters results by default?

@fgarit-te
Copy link

Has anyone investigated wether Pinia cache getters results by default?

Yes, and they do.

@yyx990803
Copy link
Member

yyx990803 commented Oct 14, 2022

closed via #1883 - will release patch minor soon (note the new minor will require Vue 3.2+).

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