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

.volatile() deprecation issues #18011

Closed
kanongil opened this issue May 6, 2019 · 8 comments
Closed

.volatile() deprecation issues #18011

kanongil opened this issue May 6, 2019 · 8 comments

Comments

@kanongil
Copy link
Contributor

kanongil commented May 6, 2019

Opening this, since my comments in #17709 (comment) seems to have been ignored.

I have 3 use cases that aren't covered by the deprecation guide.

Case 1: Class extend inheritance

const A = Component.extend({
  foo: computed(function() {
    return { foo: true };
  }).volatile()
});

const B = Component.extend({
  foo: computed(function() {
    let obj = this._super();
    obj.bar = 123;
    return obj;
  }).volatile()
});

Case 2: Relying on dependent properties

Example:

  currentLimitMs: computed('program.{start,duration}', function () {
    cancel(this._invalidateNow);

    let start = this.get('program.start');
    let duration = this.get('program.duration');

    let now = Date.now();
    let limitMs = now - start;
    if (limitMs < duration) {
      this._invalidateNow = later(() => this.notifyPropertyChange('currentLimitMs'), 1000);
    } else {
      limitMs = duration;
    }

    return limitMs;
  }).volatile(),

Case 3

The final case is for an internal addon, which exposes a ComputedProperty instance that needs to be able to call the _getter on all get() calls. This case fundamentally clashes with the deprecation.

Any chance that the deprecation can be reverted?

@kanongil
Copy link
Contributor Author

@pzuraq You did the RFC for this deprecation. Can I get a comment?

@pzuraq
Copy link
Contributor

pzuraq commented May 22, 2019

This deprecation cannot be reverted, we're moving away from computed properties in general and bringing them inline with standard native getters and this is part of that process.

In the first case, that is unfortunate. You can use the super keyword in native classes to achieve the same effect:

class A extends Component {
  get foo() {
    return { foo: true };
  }
}

class B extends A {
  get foo() {
    let obj = super.foo;
    obj.bar = 123;
    return obj;
  }
}

Since native classes will be the default by the time we remove .volatile(), if you still need this functionality then this will be the recommended way of doing it.

For the second case, that doesn't really make sense. Adding a dependent key to a volatile computed does nothing, since the computed only recomputes when it is accessed, and recomputes every time it is accessed, so dependent keys do nothing (this should actually probably throw an error tbh).

I'm not sure what you mean by the third case, can you provide an example?

@kanongil
Copy link
Contributor Author

A quick comment:

It is a bit convoluted but the second case does do something. It triggers any currentLimitMs-dependent property to update on the listed dependent key changes. Otherwise, secondary dependencies are never update unless notifyPropertyChange() is manually called.

I'll get back to you on the other points.

@pzuraq
Copy link
Contributor

pzuraq commented May 23, 2019

@kanongil actually, it shouldn't. volatile computed properties never send property change notifications, that's the point behind them. They're blackholes that are fully unobservable.

For reference, this is where a computed will normally trigger property notifications. We short circuit in volatile computeds here and never notify.

@sandstrom
Copy link
Contributor

I'm doing some issue gardening 🌱🌿 🌷 and came upon this issue. Since it's quite old I just wanted to ask if this is still relevant? If it isn't, maybe we can close this issue?

By closing some old issues we reduce the list of open issues to a more manageable set.

@locks
Copy link
Contributor

locks commented Oct 13, 2021

volatile() is deprecated and I believe being removed in 4.0, so I am closing this issue as no longer relevant.

@locks locks closed this as completed Oct 13, 2021
@kanongil
Copy link
Contributor Author

This issue is exactly about problems with the deprecation, and should be even more relevant given that you can no longer fall back to it.

@chriskrycho
Copy link
Contributor

As far as I can tell:

  1. This is perfectly doable in native class syntax, as @pzuraq described!
  2. Here you were using @computed syntax, but the reason it can't just be a plain getter is that you were actually overloading it to both return a value (real CP) and using that CP as an observer. (The use of notifyPropertyChange gives away the latter.) While I would recommend rewriting this entirely into other patterns, you can still just use an actual observer for it unless or until observers are deprecated. For what it's worth, we've found that removing observer-style patterns has consistently and often dramatically improved the clarity and maintainability of our code!
  3. I don't think you ever actually provided elaboration on what you meant by the third note. Did you resolve those in other ways?

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

5 participants