-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Merged
rwjblue
merged 3 commits into
emberjs:master
from
pzuraq:deprecate-computed-property-modifier
Oct 26, 2018
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
- Start Date: 2019-09-13 | ||
- RFC PR: (leave this empty) | ||
- Ember Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Deprecate the computed `.property()` modifier which can be used to add dependent | ||
keys to computed properties. | ||
|
||
# Motivation | ||
|
||
Currently, computed properties can use the `.property` modifier to add dependent | ||
keys to a computed _after_ the computed has been declared: | ||
|
||
```js | ||
foo: computed('strings', { | ||
get() { | ||
return this.strings.filter(s => s.includes(this.filterText)); | ||
} | ||
}).property('filterText') | ||
``` | ||
|
||
In most cases, this ability is redundant, since the dependent keys can be moved | ||
into the original computed declaration and be equivalent: | ||
|
||
```js | ||
foo: computed('strings', 'filterText', { | ||
get() { | ||
return this.strings.filter(s => s.includes(this.filterText)); | ||
} | ||
}) | ||
``` | ||
|
||
The one exception is in the case of computed _macros_, specifically macros which | ||
accept a _function_ such as `filter()` and `map()`: | ||
|
||
```js | ||
foo: filter('strings', function(s) { | ||
return s.includes(this.filterText); | ||
}).property('filterText') | ||
``` | ||
|
||
The issue stems from the fact that the inner function can access the class | ||
instance and use dynamic properties from it, and this access is opaque to the | ||
macro. | ||
|
||
This API is confusing since it bears a strong resemblance to the older style | ||
of computed property declarations, and at first glance appears to be invalid. | ||
The few edge-case macros where it does legitimately apply can be rewritten to | ||
accept more dependent keys, making it fully redundant. | ||
|
||
# Transition Path | ||
|
||
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 RFC proposes that these macros be updated to receive additional dependent | ||
keys via their public API directly via an optional second parameter which is an | ||
array of the keys: | ||
|
||
```ts | ||
function filter(filteredPropertyKey: string, callback: Function): ComputedProperty; | ||
function filter( | ||
filteredPropertyKey: string, | ||
additionalDependentKeys: string[], | ||
callback: Function | ||
): ComputedProperty; | ||
|
||
function map(mappedPropertyKey: string, callback: Function): ComputedProperty; | ||
function map( | ||
mappedPropertyKey: string, | ||
additionalDependentKeys: string[], | ||
callback: Function | ||
): ComputedProperty; | ||
``` | ||
|
||
## Deprecation Timeline | ||
|
||
The deprecation should follow these steps: | ||
|
||
* Update `filter` and `map` to their new APIs | ||
* Add a deprecation warning to uses of `.property` which add dependent keys to | ||
computed properties. | ||
* Add an optional feature to turn the deprecation into an assertion | ||
* After enough time has passed for addons and users to update, enable the | ||
optional feature by default in new addons and apps | ||
* Fully remove `.property()` in Ember v4.0.0 | ||
|
||
# How We Teach This | ||
|
||
In most cases, we shouldn't have to teach anything. There are already linting | ||
rules prohibiting `.property()` usage, and the recommended path is to provide | ||
all dependent keys in the original declaration of the computed property. For | ||
users of `map` and `filter` we should ensure that they new documentation is | ||
clear on how to add dependent keys to either macro. | ||
|
||
For addon authors that have created their own macros which rely on callbacks and | ||
have similar issues, we should demonstrate how they can structure their macro | ||
API to accept additional dependent keys. | ||
|
||
# Drawbacks | ||
|
||
The new proposed APIs for `filter` and `map` may be somewhat confusing, since | ||
only the first argument will be filtered/mapped | ||
|
||
# Alternatives | ||
|
||
We could allow additional dependent keys to be passed via an options argument: | ||
|
||
```ts | ||
filter(dependentKey: string, options: { | ||
additionalDependentKeys: [], | ||
callback: Function, | ||
}): ComputedProperty; | ||
|
||
map(dependentKey: string, options: { | ||
additionalDependentKeys: [], | ||
callback: Function, | ||
}): ComputedProperty; | ||
``` | ||
|
||
This is more verbose, but would be very clear. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but couple comments:
It looks like all
ComputedProperty
instances attach theirdependentKey
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?
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 foradditionalDependentKeys
is unnecessary?https://github.com/emberjs/ember.js/blob/master/packages/%40ember/object/lib/computed/reduce_computed_macros.js#L56
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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:
The
filter
macro has no way to know that it should update wheneverfilterText
has changed, it only knows that whenever thestrings
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 aremap
andfilter
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, that makes sense, thanks for explaining.
For what it's worth, usingfilter().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 usingEmber.computed
for this case insteadfilter/map receiven
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 sectionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any breaking changes proposed here? Existing syntaxes should be completely compatible with a deprecation warning, have we missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, I was referring to removing
.property()
, but you're right there's a different rfc for that I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the different RFC deprecates function prototype extensions, which are actually subtly different: