Deprecate Computed Overridability and .readOnly()

[pzuraq]

rendered

[mike-north]

Have you considered other terms instead of "clobber"? Maybe

• replace
• override

[pzuraq]

override seems like a more explanatory term without context. Maybe also overload. I'll update the language of the RFC

[mike-north]

might be easier to just put a dummy term in there for now. Naming is hard, but very important in this case

[mike-north]

I really like this proposed change! Yet one more place where we're adjusting defaults to set as many developers and use cases as possible up for success.

decorators

have we considered this ⬇️ option for stuff that chains off of the return of Ember.computed() like .readOnly() or .clobberable()?

@computed.foo.bar
get myProp() { }

Personally, I find something like this to be less readable

@foo
@bar
@computed
get myProp() { }

[rwjblue]

have we considered this ⬇️ option for stuff that chains off of the return of Ember.computed() like .readOnly() or .clobberable()?

@computed.foo.bar
get myProp() { }

Yep, @pzuraq and I discussed this yesterday actually. I think our conclusion was something like what you wrote:

@computed('first', 'last').thing()
get name() {}

[NullVoxPopuli]

👍 yay! 😄

[pzuraq]

To expand on that, we definitely want to add .readOnly() to ember-decorators today to match existing computed semantics. What is more of an open question is whether we should add .overridable() when we remove .readOnly(). On the one hand, as you pointed out it is easier to read, but on the other hand its somewhat orthogonal to decorator composability.

Consider the computed macro @and. It really is providing 2 things:

1. A caching layer (the same one as @computed)

2. A getter which looks like:

   get [key] { 
     return this[depKey1] && this[depKey2];
   }

The fact that these two things ended up together is kind of a historical accident, because the easiest way to define and use a macro was through creating a computed property. With decorators though, that changes, we can fairly easily compose functionality like caching, notifying, and macros. In the future, we're going to introduce a new system based around references, @tracked.

Now, unfortunately with the current state of computed macros, there won't be an easy way to use @tracked with macros without incurring the cost of @computed. If we were to separate out these two functions into separate decorator systems though, it could look something like this:

class Foo {
  @computed @and('bar', 'baz') foo;
  
  @tracked @and('bar', 'baz') foo2;
}

The definition of the macro itself is separate from the meta information (caching provided by @computed, notifications provided by @tracked). This also has the benefit that the macro system would no longer be tied to Ember in any way - it could be a generic JS decorator library.

In the case of @overridable, it would provide a self-destructing setter function which would be composable with any macro which provides a getter function. That said, this is more about the future of computed macros (and even further out, @tracked and how we'll adopt that system) so I think adding .overridable() would also make sense, and we could consider redesigning it whenever we do reimplement macros for the @tracked world.

[pzuraq]

@mike-north I actually just went over the spec again and realized we're more limited than I thought. Decorators can only be chains of properties with one final call at the end:

// valid
@foo
@foo.bar
@foo()
@foo.bar()
@(foo().bar)
@(foo().bar())

// invalid
@foo().bar
@foo().bar()
@foo()()

This is going to affect readOnly and volatile too unfortunately 😕

[mike-north]

This syntax is still valid. Remember we're just capturing dependent keys and flagging the property in some manner to indicate readonly-ness. We only need to capture arguments once

class {
   @computed.volatile.readOnly('firstName', 'lastName')
   get fullName() {
		...
   }
}

There are some very popular projects that have chaining like this, albeit in a conventional function call instead of a function that returns a decorator.

[balinterdi]

@pzuraq Thank you for putting this together.

While we still can't just use ES classes and decorators in Ember (or can we already?) how would we replace the following if this RFC gets implemented (since it would deprecate computed overridability):

  showErrors: computed('_showErrors', {
    get() {
      return this._showErrors || { email: false, password: false };
    },
    set(key, value) {
      this.set('_showErrors', value);
      return this._showErrors;
    }
  }),

(The transition path shows an example but it uses an ES class.)

Thank you.

[pzuraq]

Hey Balint, the example you posted would be perfectly valid since the computed property provides a setter. What would not be valid is this:

const Foo = EmberObject.extend({
  showErrors: computed('_showErrors', {
    get() {
      return { email: false, password: false };
    }
  })
});

let foo = Foo.create();

foo.set('showErrors', { email: true, password: true }); // Throws an error because `showErrors` does not have a setter

Does that make sense?

[balinterdi]

@pzuraq Yes, it does and I then wholeheartedly support this RFC :) Thank you.

[Gaurav0]

I don't object to the spirit of this RFC, but I need to point out that this is a breaking change and will likely mess up poorly written legacy apps. I strongly advise that we throw a deprecation warning when calling set() on a computed property without a setter, and then make them readonly by default in 4.0.

This unfortunately means we need to continue to clobber during the 3.0 lifecycle.

[pzuraq]

@Gaurav0 that was the original intent of this RFC, but the deprecation aspect of it was only implied by the title and the summary. I'll add an extra section to the Transition Path section to clarify that the transition would follow the standard deprecation path for breaking changes, showing a warning until v4.0.0

[Gaurav0]

If we deprecate this, users have no way of getting a hard error (not a deprecation warning) when they clobber without getting deprecation warnings by using readOnly.

I recommend only deprecating .readOnly() in the 4.x era. Yeah, like I said, this is a pain.

[rwjblue]

The core team met today and discussed this, we are in favor of moving this forward into final comment period.

If you haven't read through the RFC just yet, now is a great time to check it out!

[rwjblue]

We chatted about this today in the core team meeting, and decided to try to leverage svelte here instead of an optional feature. This doesn't change the timeline at all here, just the verbiage.

Can you tweak this from Add optional feature to When the application is configured with svelte, the deprecation will be an assertion?

[webark]

@pzuraq did the overridable computed macro ever get into an addon somewhere?

[pzuraq]

It did not, but it should be relatively easy to write to interop with something like macro-decorators. I don't think it'd be easy to write to interop with @computed, unfortunately, due to the mandatory setter.

[alexlafroscia]

I ended up building an overridable macro as an addon here:

https://github.com/alexlafroscia/ember-overridable-computed

It doesn't support being used as a decorator yet, but I'll do that eventually!