Make computed properties cacheable by default

[mattetti]

I've been fighting a bug for a few hours when I realized that my expectations of computed properties were wrong.
Basically, I was creating an array in a computed property, then I was rendering each collection element. When click on, the element view would modify itself, but the computed property was not updated which drove me crazy.

I was under the (wrong) assumption that using a computed property would NOT recreate a new object every time. The solution was of course to use cacheable() on my computed property.

Please consider using cacheable by default since it would probably save a lot of frustration.

[drogus]

I think that when you do it cacheable by default, someone will create another ticket saying that it should not be cached by default and it would save a lot of frustration to make it so ;)

[mattetti]

I have to disagree for a few reasons:

• how often/why would you create a computed property that relies in the creation of new objects every time a dependency is changed?
• it should be quite straight forward to add another method to avoid the caching mechanism and let people opt out of this feature.
• the expectation of a new user is that using property() keeps the content of the function in sync with other dependencies. Now, that's really only true if using cacheable() on the computed property that creates objects, because otherwise previously defined properties related to the non cached computed property objects will point to an older version of the object still in memory. Because the refence still exists, the GC won't collect the object and there is no easy way to know that the reference is not what one might expect.

So I would say that or the change I suggested is implemented or a warning/error is raised when a property points to an old reference. Note that the second option is much harder to implement and probably quite messy.

[erichocean]

FWIW, You can easily tweak SproutCore before your own code loads to make this the default. All cacheable() does is flip a flag on the function.

Nevertheless, I too agree that cacheable() should be the default in SproutCore 2.

[lukemelia]

+1 to making cacheable the default.

Pretty much the only time I don't include it is when I forget to.

[ColinCampbell]

+1 as well. People will still need to know the difference as there are cases where you want it turned off, but I think that needs to be left up to the developer. Most computed properties should be cacheable IMO; thus it should be the default.

[ef4]

Yes, definitely make these cacheable by default. It is by far the more common case. I've been sticking a preamble onto my projects that does this. If you count real uses in real projects, cacheable things are far more common.

I'm mostly swayed by the fact that the more common case should be brief.

I'm less convinced by the "helping new users" argument, because neither expectation is more obviously correct than the other, and there are potentially frustrating subtleties either way. What's needed for new users are good docs and a responsive community.

[roydaniels]

+1 on making cacheable the default as well. A simple but much needed change.

[robmonie]

+1 - It seems like a sensible default to me. It's definitely been the rule rather than the exception for us.

[wagenet]

@wycats, @tomdale: it would be good to get your input on this. We'd probably have to adjust the cacheable helper to take false as a param to turn off caching.

[wagenet]

The biggest concern with this is that it would be a breaking change for some apps. We might want to also provide an ENV variable to change the default to make it easier for old users to maintain the previous behavior, though I think we'd eventually want to remove that option to get everyone on the same page.

[ef4]

Ember is still young enough that breaking changes are appropriate. It's only going to get harder later.

I seriously doubt an app written against SC1.6beta (the first fork from Sproutcore 1.x) would run without adaptation against the current Ember. There have already been plenty of breaking changes. That is entirely appropriate. Better to make them now.

[juan77]

+1 to the idea of making cacheable by default, BUT IMHO when properties have dependencies, so if no dependencies, make it a non cacheable property. I think that is more common sense, so:

).property(‘firstName’, ‘lastName’) -> cacheable by default
).property() -> no cacheable by default

[ef4]

I'm not in favor of making the cacheability vary depending on whether there
are dependencies. For one thing, it's more complicated to remember. For
another, my own dependency-less properties are more often cacheable (either
static or explicitly expired elsewhere by notifyPropertyChange).

On Jan 16, 2012 5:42 AM, "juan77" <
reply@reply.github.com>
wrote:

+1 to the idea of making cacheable by default, BUT IMHO when properties
have dependencies, so if no dependencies, make it a non cacheable property.
I think that is more common sense, so:

).property(firstName, lastName) -> cacheable by default
).property() -> no cacheable by default

---

Reply to this email directly or view it on GitHub:
#38 (comment)

[krisselden]

On a similar note, I feel the same way about Binding.oneWay, I find it rare that I need bindings to be 2 way.

[wagenet]

@kselden I disagree on oneWay bindings. I think bindings by their nature imply two way updating so that should be the default, even if it's a bit less performant. However, if you'd like to discuss this further, please open a new ticket so that we don't hijack this discussion.

[wycats]

Circling back around here, I think this is a plausible idea, but I'm somewhat worried about unexpected consequences. I would like someone to try to write a patch that adds this behavior, fixing all the internal cases it would break (it would likely require .cacheable(false) or something similar).

That exercise would give us better insight into the cases where cacheable by default would cause problems. Once that patch is done, I personally volunteer to try to use it on my Ember projects and see what exactly fails. That will give us further insight.

In general, I agree that now is the time to do this, if we're ever going to do it. I'm closing this ticket in anticipation of a pull request that implements this feature, as I don't think there is any more possible discussion on this topic until we have more information in the form of code.

[wycats]

fwiw, the major infinite loop bug that this was trying to address is now fixed at 626d23f

[wagenet]

Whether we change the default or not, we should resolve this issue prior to the 1.0 release.

[tomdale]

I think we should just bite the bullet and do it.

[wagenet]

@tomdale How should we turn it off? Also, should we enable it only with an ENV flag at first and make it default in 1.0?

[krisselden]

@wagenet #463 (comment)

[wagenet]

In progress here: https://github.com/emberjs/ember.js/tree/cacheable-default