Deprecate computed().volatile()

[pzuraq]

rendered

[mike-north]

Because the only path forward involves using native classes, does the deprecation of .volatile() need to be tied to the deprecation of Ember.Object.extend({}) style syntax? As long as we support ES5-style syntax (I've heard nothing about this going away) we should also support a deprecation-free way to get a volatile-ish thing.

That being said, I'm a fan of moving in this direction for most apps. Maybe all this needs is a plan for an addon to opt back in to the old behavior, with no deprecation warnings.

looks like there's an ES5 path forward: #370 (comment) Thanks for pointing this out @rwjblue

[pzuraq]

If we do not provide some other way to declare volatile properties, then yes it would have to be available until ES5 classes are deprecated and removed. I have proposed a timeline for discussion in #338, but it really only sets a lower bound on removal - the earliest we could completely remove them is by Ember 5.

@rwjblue pointed out a way we could support getters/setters in ES5 syntax without modification below, so we won't need to tie them together.

[rwjblue]

Because the only path forward involves using native classes, does the deprecation of .volatile() need to be tied to the deprecation of Ember.Object.extend({}) style syntax?

No 😸, and native classes aren't the only way forward (though the alternatives are less ergonomic than ES classes).

For example, you can migrate from:

export default Component.extend({
  init() {
    this._super(...arguments);
    this._foo = 0;
  },

  foo: computed(function() {
    return this._foo++;
  }).volatile()
});

To:

const FooBar = Component.extend({
  init() {
    this._super(...arguments);
    this._foo = 0;
  }
});

Object.defineProperty(FooBar.prototype, 'foo', {
  get() {
    return ++this._foo;
  }
});

export default FooBar;

[mike-north]

TODO: update this to clarify that getter/setter based properties with Object.defineProperty still work with older versions of Ember.Object

[tomdale]

I'm 👍 on moving away from volatile semantics, but my only concern is about the timing of this deprecation. If there are use cases where people are legitimately using .volatile(), I think they will feel (correctly) like they are being pushed towards native class syntax. The proposed workaround of Object.defineProperty feels pretty bad in comparison. Are we confident that the majority of classes can be moved to native syntax if need be?

Ultimately, this functionality is so rarely used I don't think it really matters either way. We should just make sure that the O.dP workaround is not going to cause people to flip tables. FWIW, I only found three uses of it in the entire linkedin.com Ember app, although I may have missed some usage in our dependency graph.

[tomdale]

We discussed this at the core team meeting today and are in favor of moving this deprecation to FCP.

One of the concerns raised was whether the migration path away from volatile was "ready" yet, or if we should wait until things like native JS classes are more fully adopted so it's easier to migrate this behavior to native getters.

Ultimately, we believe that this feature is very rarely used, and when it is used, it is often used incorrectly. The specific semantics of volatile() are confusing, and usually misunderstood by people who pick it up as a tool. We also believe that almost all cases using .volatile() can be better modeled with other API.

For example, one use case people sometimes use volatile() to solve is time-based computed properties. For example, there might be a CP like this:

now: computed(function() {
  return Date.now();
}).volatile()

However, this modeling is not ideal because if this value is used in a template, the template will render once and never update again. A better way to model this is to use a timer to set a static property on a regular basis:

init() {
  setInterval(() => {
    this.set('now', Date.now());
  });
}

Now other CPs can depend on this now property and still be invalidated and updated correctly if they're used in a template.

For cases that are truly volatile but pull-based (i.e. they are never going to be used in a template or as a dependent key for a CP that will be used in a template, but called directly only when needed), they should be turned into either a method or a native getter. Because CPs invalidate and update automatically in 99% of cases, using a CP for something that is actually volatile gives downstream consumers of that property a false intuition about its behavior.

One annoying limitation in the current EmberObject.extend() implementation is that it's not ergonomic to define native getters. To add a native getter to one of these classes, developers can use Object.defineProperty (as noted in the discussion above). An alternative workaround is to create a second subclass via native syntax, e.g.:

const _MyClass = Ember.Object.extend({
  // ...
});

class MyClass extends _MyClass {
  get now() {
    return Date.now();
  }
}

Because of how rare real world use of volatile() is, we are comfortable with these less-than-ergonomic workarounds. If anyone objects to the verbosity, they can always disable the deprecation and wait until they move the class to native class syntax. But, to reiterate, we believe the problem can usually be modeled in a better way anyway. Let's just make sure we have a good deprecation guide that outlines the alternatives.

[dgeb]

With no further comments, we decided to merge this in the core team meeting today.

Thanks @pzuraq! 🎉