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

Make computed property caching optional #1189

Closed
karevn opened this issue Aug 20, 2015 · 23 comments
Closed

Make computed property caching optional #1189

karevn opened this issue Aug 20, 2015 · 23 comments

Comments

@karevn
Copy link

karevn commented Aug 20, 2015

"computed" properties were re-calculated on any change in past. Now, they're cached, and the algorithm used does not make sense for me (and most of other people, I'm sure). At least, it never flushed and updated computed properties in my project, being actually useless. So, I think having a good documentation or flush algorithm that would match "common sense" is a must for this feature.

@yyx990803
Copy link
Member

Please provide a reproduction where it doesn't work as intended.

@GirlBossRush
Copy link

Consider the following, a paginated list that can have multiple columns.

function entitiesPerViewport () {
  // The paginated list is a child of a scrollable container.
  var clientHeight = this.$parent.$el.clientHeight;

  return this.columns * Math.round(clientHeight / this.entityHeight);
};

Prior to the change in computed property evaluation, this always worked. Now it must be placed in methods. The docs state,

...computed properties are cached and lazily re-evaluated only when necessary.

but offer no explanation to the conditions that invalidate the cache. I suspect that only changes to tracked $data invalidates the cache.

@yyx990803
Copy link
Member

Hmm, I guess this is because I personally only use computed properties for making the view declarative - i.e. I bind to it in the template, but never rely on it in imperative code. It was never meant to be used as a getter, but I guess a lot of people do.

@TeffenEllis you are right that the cache only invalidates when a reactive dependency has changed. Vue has no way of knowing when an external dependency like clientHeight has changed.

I can make the cache optional, since it only makes a performance difference when the computed property is unusually expensive.

@yyx990803 yyx990803 changed the title Clean "computed" data flush strategy. Make computed property caching optional Aug 20, 2015
@GirlBossRush
Copy link

@yyx990803,

I believe that adding a config to making computed property cache optional might be unnecessary. Clarification in the documentation can help the user understand when a computed property is appropriate over a getter function.

Computed properties are cached and re-evaluated only when a reactive dependency has changed.

Thank you!

@yyx990803
Copy link
Member

@TeffenEllis It could be done in a very unobtrusive way. By default it will work like before (without caching). If you really need caching for some specific scenario, you can enable it on a per-property basis:

computed: {
  a: function () {
    // getter-like computed
  },
  b: {
    cache: true,
    get: function () {
      // cached, only re-evaluates when a reactive dependency changes
    }
  }
}

@GirlBossRush
Copy link

+1 on a per computed configuration, defaulting to true.

@karevn
Copy link
Author

karevn commented Aug 21, 2015

@yyx990803
Why not to make one step further?

computed: {
  a: function () {
    // getter-like computed
  },
  b: {
    cache: function() {
        //check if the property is outdated
    },
    get: function () {
      // cached, only re-evaluates when a reactive dependency changes
    }
  }
}

Plus, boolean cache value for always-cached or always-re-evaluated properties.

@yyx990803
Copy link
Member

@karevn I don't really see the point for making cache a function. For me it just makes it unnecessarily complicated.

@karevn
Copy link
Author

karevn commented Aug 22, 2015

@yyx990803 the "hard" way with the cache function is a bit overkill, but... it may be helpful for complex cases. Why not to make it work both ways: if cache is a function - then use its result to mark cache record as stale. If just a boolean - use it to enable/disable caching?

@azamat-sharapov
Copy link

I think making cache accept function will make it confusing and people may end up creating slow apps, if they don't use it correctly. Before, there was no cache thing for computed properties and I remember somebody complaining about performance and Evan advising him not to use heavy operations inside computed properties. After that he implemented caching.

@karevn
Copy link
Author

karevn commented Aug 22, 2015

If most of computed properties are only evaluated once - what is the point of having them at all? It's easier to just make one computation at data function or in ready handler. So, computed properties should be smarter than that, or removed alltogether for sake of simplicity. I tried to use them in their current state and ended up using other approaches (re-computed data or methods), because could not control caching strategy effectively enough. So IMHO, the question is "how to make it easy to use for newbies and powerful enough for advanced usage". In other words - make them aggressively cached by default, with Evan's "cache" option, and optional function for "cache" for most advanced usage. For example - in my current app there are lots of computations related to window object, and some DOM element sizes. It would be VERY useful to have a control over caching with one function passed to several computed properties. Or at least have an API to mark cached computed value as "stale".

@yyx990803
Copy link
Member

@karevn I don't think you really understand what computed properties are for. It's not a "automagic always up-to-date" property - it's used to compute value based on other reactive values. If you are putting a lot of window/DOM sizes inside computed properties and expect them to act as dependencies, I'd say you are using it wrong, and that's why you find it "not useful".

@karevn
Copy link
Author

karevn commented Aug 22, 2015

@yyx990803 Got it. But... if all the computations are based on vue's reactive values only, then they're pretty cheap, and why bother with caching them at all?

@yyx990803
Copy link
Member

@karevn it's cheap most of the time, but you can also, say, have a computed property that map/filter/reduces a HUGE array, which can be slow. Then let's suppose you have additional computed properties that rely on this one... you'd be calling the expensive computed property many more times than necessary.

@karevn
Copy link
Author

karevn commented Aug 22, 2015

@yyx990803 Agree. But... I still think adding more control over caching will not hurt. :)

@karevn
Copy link
Author

karevn commented Aug 24, 2015

@yyx990803 I meant function value for cache option. And, just to make sure there's no confusion: this function's return value should determine when does the cache flush (i.e. it returns true - keep it cached for now, false - triggers a re-calculation).

@yyx990803
Copy link
Member

@karevn but that would not be very useful, because how do we know when to call that function? If we only call it when a reactive dependency changes, then it's not really different from cached.

@adrianb93
Copy link

adrianb93 commented Nov 22, 2016

Was this implemented in Vue 2.0? My computed properties aren't updating when a shared store is updated. Computed properties are needed for v-model attribute.

In my situation, the shared store contains the root node (render pipeline manager) which render function creates new elements passing the previous element in the pipeline in as a slot (essentially nested components all referencing the same store for their property). The computed property that is referenced in a "v-model" attribute updates the root node's data via the store. This then trickles down the render pipeline exactly as expected, however, the computed properties in all child nodes remain as the same value.

Not sure if this is a bug in Vue or a good use case for computed property cache busting.

Update:

This issue also exists for the root node's computed properties, made those into methods to avoid this issue, however, the child nodes still rely on the computed property as described above.

@yyx990803
Copy link
Member

@adrianb93 this was later reverted. Caching is now enabled by default for computed properties. The behavior you are describing is mostly like because your shared store isn't made reactive, but your description is still to vague to determine the real cause. If you think it's a bug you should be opening a separate issue with proper reproduction.

@mesqueeb
Copy link

mesqueeb commented Feb 25, 2017

@yyx990803
Is there a way to force a recalculation of a computed property? e.g. flushing the cache.

@posva
Copy link
Member

posva commented Feb 25, 2017

@mesqueeb please, don't ask question on issues, even less on closed ones. Use the Forum instead. You can use a method (https://vuejs.org/v2/guide/migration.html#cache-false-deprecated)

@MrCoder
Copy link

MrCoder commented Dec 16, 2017

@adrianb93 Have you solved your problem? I think I may be facing the same problem. When I use a single Store, the computed properties are reactive to the Store's getters and when I use the Store as a module, it is not reactive any longer. I have been stuck on this for a few days.
-- Update
It has nothing to do with sub-modules. Will update when I find more.

@AllanOricil
Copy link

AllanOricil commented Aug 28, 2021

For some reason my computed properties that are based on Vuex state are not cached. Im not using a Vuex getter that is a function, so it should be cached. But it is being called all the time. Look at the code below. There are two dependencies in the apiVersions computed prop, the salesforceApiVersions, which comes from the store, and the username, wich is a local data. The salesforceApiVersions does not change, nor the username, but Vue is recomputing the apiVersions every time a spawn a new instance of this component (it runs once for the new component instance, and run again for the older component instance that was already mounted). I really don't know why this happening as it is not doing what stated in the docs.

computed: {
        ...mapState({
            defaultApiVersion: (state) => state.salesforce.defaultApiVersion,
            salesforceApiVersions: (state) => state.salesforce.apiVersions
        }),
        state(){
            return this.$store.state[this.editorName]
        },
        apiVersions(){
            return this.salesforceApiVersions[this.username] ? this.salesforceApiVersions[this.username]
                    .map((apiVersion) => {
                        return 'v' + apiVersion.version
                    }) : []
        },
    },

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

No branches or pull requests

9 participants