vuejs / vue

This is the repo for Vue 2. For Vue 3, go to https://github.com/vuejs/core
http://v2.vuejs.org
MIT License
208.02k stars 33.69k forks source link

Make computed property caching optional #1189

Closed karevn closed 9 years ago

karevn commented 9 years ago

"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 commented 9 years ago

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

GirlBossRush commented 9 years ago

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 commented 9 years ago

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.

GirlBossRush commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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

karevn commented 9 years ago

@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 commented 9 years ago

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

karevn commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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

karevn commented 9 years ago

@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 commented 9 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

posva commented 7 years ago

@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 commented 6 years ago

@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 commented 3 years ago

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
                    }) : []
        },
    },