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

Deprecate Computed `.property()` Modifier #375

Open
wants to merge 3 commits into
base: master
from

Conversation

@pzuraq
Contributor

pzuraq commented Sep 14, 2018

Rendered

@pzuraq pzuraq changed the title from Deprecated Computed `.property()` Modifier to Deprecate Computed `.property()` Modifier Sep 17, 2018

@locks locks added this to Deprecations in Deprecation Candidates Sep 18, 2018

@locks locks moved this from Deprecations to In Progress in Deprecation Candidates Sep 18, 2018

@cibernox

This comment has been minimized.

Show comment
Hide comment
@cibernox

cibernox Sep 21, 2018

Contributor

Completely in favor of this. It's super weird. I offer to help with the deprecation if approved.

Contributor

cibernox commented Sep 21, 2018

Completely in favor of this. It's super weird. I offer to help with the deprecation if approved.

@simonihmig

This comment has been minimized.

Show comment
Hide comment
@simonihmig

simonihmig Sep 24, 2018

Contributor

Agree as well!

Mainly bike shedding here, but what about this function signature:

filter(filteredPropertyKey: string, callback: Function, additionalDependentKeys?: string[]): ComputedProperty;

I would prefer it strongly over the one mentioned in "Alternatives", and on par with the one proposed. Maybe a tad easier to refactor existing code, by moving the additional dependent keys form .property() to the third optional argument?

Contributor

simonihmig commented Sep 24, 2018

Agree as well!

Mainly bike shedding here, but what about this function signature:

filter(filteredPropertyKey: string, callback: Function, additionalDependentKeys?: string[]): ComputedProperty;

I would prefer it strongly over the one mentioned in "Alternatives", and on par with the one proposed. Maybe a tad easier to refactor existing code, by moving the additional dependent keys form .property() to the third optional argument?

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Sep 24, 2018

Contributor

@simonihmig my only concern with that API is looking forward to usage with decorators. Currently, we allow these macros to use the function they are being applied to as the filter function in ember-decorators:

@filter('key', function() {
  // filter code...
}) 
filterProp;

// or

@filter('key')
filterProp() {
  // filter code...
}

In this usage, the function always comes last lexically, so we'd end up with:

@filter('key', ['additional', 'keys'])
filterProp() {
  // filter code...
}

I think the array makes it pretty readable though, so I wouldn't be opposed to changing at all. Just need to factor in that these will probably be changing soon. Also, decorator APIs are not set in stone, and still need to be RFCd, so that form could be removed in the future.

Contributor

pzuraq commented Sep 24, 2018

@simonihmig my only concern with that API is looking forward to usage with decorators. Currently, we allow these macros to use the function they are being applied to as the filter function in ember-decorators:

@filter('key', function() {
  // filter code...
}) 
filterProp;

// or

@filter('key')
filterProp() {
  // filter code...
}

In this usage, the function always comes last lexically, so we'd end up with:

@filter('key', ['additional', 'keys'])
filterProp() {
  // filter code...
}

I think the array makes it pretty readable though, so I wouldn't be opposed to changing at all. Just need to factor in that these will probably be changing soon. Also, decorator APIs are not set in stone, and still need to be RFCd, so that form could be removed in the future.

@lupestro

This comment has been minimized.

Show comment
Hide comment
@lupestro

lupestro Sep 26, 2018

Contributor

I really like the array form for the decorator, as it should be really easy to document and teach.

@filter('key', ['additional', 'keys'])
filterProp() {
  // filter code...
}

Would the following be disallowed?

filter(filteredPropertyKey: string, additionalDependentKeys?: string[], callback: Function) : ComputedProperty;

This variation on the proposed alternative makes it very clear that the first key is required and delineates its role clearly. It is more readable because the function is last, so you aren't having to walk back across a function body from other parameters to find the beginning of the call. (APIs always carry less "cognitive load" if you can observe and dismiss all the "short" parameters with a single glance before digging into the meat of the function.) It would work well with syntax completion, and would be very easy to teach. It also happens to be the pure functional equivalent of the array form of the decorator.

Contributor

lupestro commented Sep 26, 2018

I really like the array form for the decorator, as it should be really easy to document and teach.

@filter('key', ['additional', 'keys'])
filterProp() {
  // filter code...
}

Would the following be disallowed?

filter(filteredPropertyKey: string, additionalDependentKeys?: string[], callback: Function) : ComputedProperty;

This variation on the proposed alternative makes it very clear that the first key is required and delineates its role clearly. It is more readable because the function is last, so you aren't having to walk back across a function body from other parameters to find the beginning of the call. (APIs always carry less "cognitive load" if you can observe and dismiss all the "short" parameters with a single glance before digging into the meat of the function.) It would work well with syntax completion, and would be very easy to teach. It also happens to be the pure functional equivalent of the array form of the decorator.

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Sep 27, 2018

Contributor

I agree, that form is the least complicated and easiest to read IMO, plus it works really well in decorator form. I'll update the RFC accordingly, thanks for the feedback @RalphMack and @simonihmig 😄

Contributor

pzuraq commented Sep 27, 2018

I agree, that form is the least complicated and easiest to read IMO, plus it works really well in decorator form. I'll update the RFC accordingly, thanks for the feedback @RalphMack and @simonihmig 😄

As mentioned above, macros which receive a callback function as an argument are
the only valid use of `.property()` in current Ember. Currently, there are two
such macros in Ember core: `map` and `filter`.

This comment has been minimized.

@mehulkar

mehulkar Oct 2, 2018

I might be missing something here, but couple comments:

  1. It looks like all ComputedProperty instances attach their dependentKey using .property.

    https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/metal/lib/computed.ts#L617

    Does that mean that the class itself needs to be changed to accept dependentKeys as constructor arguments?

  2. Secondly, in the case of arrayMacros, it looks like the only dependentKey actually being attached using .property() is the key that's originally passed in (appended with .[]). Which to me sounds like the proposal for additionalDependentKeys is unnecessary?
    https://github.com/emberjs/ember.js/blob/master/packages/%40ember/object/lib/computed/reduce_computed_macros.js#L56

@mehulkar

mehulkar Oct 2, 2018

I might be missing something here, but couple comments:

  1. It looks like all ComputedProperty instances attach their dependentKey using .property.

    https://github.com/emberjs/ember.js/blob/master/packages/%40ember/-internals/metal/lib/computed.ts#L617

    Does that mean that the class itself needs to be changed to accept dependentKeys as constructor arguments?

  2. Secondly, in the case of arrayMacros, it looks like the only dependentKey actually being attached using .property() is the key that's originally passed in (appended with .[]). Which to me sounds like the proposal for additionalDependentKeys is unnecessary?
    https://github.com/emberjs/ember.js/blob/master/packages/%40ember/object/lib/computed/reduce_computed_macros.js#L56

This comment has been minimized.

@pzuraq

pzuraq Oct 2, 2018

Contributor
  1. This is more of an implementation detail which is why I didn't address it in the RFC, but yes this would have to change. The most direct way forward would be to make the property() api private (e.g. _property) but we could change the constructor as well.

  2. The key difference with the array macros is that they receive arbitrary functions as arguments, and have no way of knowing if those functions introduce additional dependencies. Take the example from the RFC:

    foo: filter('strings', function(s) {
      return s.includes(this.filterText);
    }).property('filterText')

    The filter macro has no way to know that it should update whenever filterText has changed, it only knows that whenever the strings array has updated it should as well. Everything that happens inside the function argument is a black box to it, completely opaque. This can affect any macro which receives arbitrary functions as an argument, it just so happens that the only ones in Ember's core library are map and filter.

@pzuraq

pzuraq Oct 2, 2018

Contributor
  1. This is more of an implementation detail which is why I didn't address it in the RFC, but yes this would have to change. The most direct way forward would be to make the property() api private (e.g. _property) but we could change the constructor as well.

  2. The key difference with the array macros is that they receive arbitrary functions as arguments, and have no way of knowing if those functions introduce additional dependencies. Take the example from the RFC:

    foo: filter('strings', function(s) {
      return s.includes(this.filterText);
    }).property('filterText')

    The filter macro has no way to know that it should update whenever filterText has changed, it only knows that whenever the strings array has updated it should as well. Everything that happens inside the function argument is a black box to it, completely opaque. This can affect any macro which receives arbitrary functions as an argument, it just so happens that the only ones in Ember's core library are map and filter.

This comment has been minimized.

@mehulkar

mehulkar Oct 2, 2018

Ah ok, that makes sense, thanks for explaining.

For what it's worth, using filter().property() does not seem like a common case to me. Considering that this is is a breaking change already, a couple other alternatives might be:

  • recommend using Ember.computed for this case instead
  • filter/map receive n string args followed by a function arg, where the first is the enumerable property, and all following string args are dependentKeys. This would be more closely aligned with the Ember.computed API.

I see the additional conversation about syntax and it covers my suggestions too, sorry for the noise!

@mehulkar

mehulkar Oct 2, 2018

Ah ok, that makes sense, thanks for explaining.

For what it's worth, using filter().property() does not seem like a common case to me. Considering that this is is a breaking change already, a couple other alternatives might be:

  • recommend using Ember.computed for this case instead
  • filter/map receive n string args followed by a function arg, where the first is the enumerable property, and all following string args are dependentKeys. This would be more closely aligned with the Ember.computed API.

I see the additional conversation about syntax and it covers my suggestions too, sorry for the noise!

This comment has been minimized.

@pzuraq

pzuraq Oct 2, 2018

Contributor

It's definitely not a common case, but ember-decorators have received reports of users using it this way (that's actually how I found out about this issue). But yes, those are both reasonable alternatives, I'll add them to the alternatives section

@pzuraq

pzuraq Oct 2, 2018

Contributor

It's definitely not a common case, but ember-decorators have received reports of users using it this way (that's actually how I found out about this issue). But yes, those are both reasonable alternatives, I'll add them to the alternatives section

This comment has been minimized.

@rwjblue

rwjblue Oct 2, 2018

Member

Considering that this is is a breaking change already

I do not see any breaking changes proposed here? Existing syntaxes should be completely compatible with a deprecation warning, have we missed something?

@rwjblue

rwjblue Oct 2, 2018

Member

Considering that this is is a breaking change already

I do not see any breaking changes proposed here? Existing syntaxes should be completely compatible with a deprecation warning, have we missed something?

This comment has been minimized.

@pzuraq

pzuraq Oct 2, 2018

Contributor

Removing computed().property() would technically be breaking which is what I think @mehulkar was referring too, but that would happen in Ember v4 and there would be a very long overlap period with the deprecation warning.

@pzuraq

pzuraq Oct 2, 2018

Contributor

Removing computed().property() would technically be breaking which is what I think @mehulkar was referring too, but that would happen in Ember v4 and there would be a very long overlap period with the deprecation warning.

This comment has been minimized.

@mehulkar

mehulkar Oct 2, 2018

yep, I was referring to removing .property(), but you're right there's a different rfc for that I think.

@mehulkar

mehulkar Oct 2, 2018

yep, I was referring to removing .property(), but you're right there's a different rfc for that I think.

This comment has been minimized.

@pzuraq

pzuraq Oct 2, 2018

Contributor

I think the different RFC deprecates function prototype extensions, which are actually subtly different:

foo: function() {

}.property('bar'), // calls a method that was added to Function.prototype

bar: computed(function() {

}).property('baz') // calls a method on the ComputedProperty class instance
@pzuraq

pzuraq Oct 2, 2018

Contributor

I think the different RFC deprecates function prototype extensions, which are actually subtly different:

foo: function() {

}.property('bar'), // calls a method that was added to Function.prototype

bar: computed(function() {

}).property('baz') // calls a method on the ComputedProperty class instance
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Oct 6, 2018

Member

Discussed this at the core team meeting yesterday, and we are in favor of moving this into final comment period.

Member

rwjblue commented Oct 6, 2018

Discussed this at the core team meeting yesterday, and we are in favor of moving this into final comment period.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment