From 78ef7513d0390882abc65c96017ad0aedfe342e5 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Thu, 13 Sep 2018 12:16:53 -0700 Subject: [PATCH 1/3] deprecated computed .property() modifier --- ...00-deprecate-computed-property-modifier.md | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 text/0000-deprecate-computed-property-modifier.md diff --git a/text/0000-deprecate-computed-property-modifier.md b/text/0000-deprecate-computed-property-modifier.md new file mode 100644 index 0000000000..f82d15b842 --- /dev/null +++ b/text/0000-deprecate-computed-property-modifier.md @@ -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 a list of dependent +keys instead of just one. The first dependent key will be the value to be +mapped/filtered, and the subsequent keys will only invalidate the cache. + +```js +foo: filter('strings', 'filterText', function(s) { + return s.includes(this.filterText); +}) + +// with ember-decorators +@filter('strings', 'filterText') +foo() { + return s.includes(this.filterText); +} +``` + +This will be the recommended approach for macros supplied by addons as well. + +## Deprecation Timeline + +Once the macros have been updated to accept an arbitrary number of dependent +keys, a deprecation warning should be added to calls to `.property()`. Removing +the functionality will be a breaking change, so it will be set to be removed +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 to filter/map in a +different way, such as via an options object: + +```js +foo: filter( + 'strings', + { + dependentKeys: ['filterText'] + }, + function(s) { + return s.includes(this.filterText); + } +) + +// with ember-decorators +@filter('strings', { + dependentKeys: ['filterText'] +}) +foo() { + return s.includes(this.filterText); +} +``` From 2bad2b15979448bf446debd421a2253b689c7bbc Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Tue, 18 Sep 2018 17:11:19 -0700 Subject: [PATCH 2/3] update adoption timeline and function signatures --- ...00-deprecate-computed-property-modifier.md | 72 +++++++++---------- 1 file changed, 32 insertions(+), 40 deletions(-) diff --git a/text/0000-deprecate-computed-property-modifier.md b/text/0000-deprecate-computed-property-modifier.md index f82d15b842..cf998acfea 100644 --- a/text/0000-deprecate-computed-property-modifier.md +++ b/text/0000-deprecate-computed-property-modifier.md @@ -55,30 +55,35 @@ 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 a list of dependent -keys instead of just one. The first dependent key will be the value to be -mapped/filtered, and the subsequent keys will only invalidate the cache. - -```js -foo: filter('strings', 'filterText', function(s) { - return s.includes(this.filterText); -}) - -// with ember-decorators -@filter('strings', 'filterText') -foo() { - return s.includes(this.filterText); -} +This RFC proposes that these macros be updated to receive additional dependent +keys via their public API directly via an optional configuration object mode. +The new API signatures would be: + +```ts +function filter(filteredPropertyKey: string, callback: Function): ComputedProperty; +function filter(filteredPropertyKey: string, options: { + callback: Function, + dependentKeys: string[] +}): ComputedProperty; + +function map(mappedPropertyKey: string, callback: Function): ComputedProperty; +function map(mappedPropertyKey: string, options: { + callback: Function, + dependentKeys: string[] +}): ComputedProperty; ``` -This will be the recommended approach for macros supplied by addons as well. - ## Deprecation Timeline -Once the macros have been updated to accept an arbitrary number of dependent -keys, a deprecation warning should be added to calls to `.property()`. Removing -the functionality will be a breaking change, so it will be set to be removed -in Ember v4.0.0. +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 @@ -99,25 +104,12 @@ only the first argument will be filtered/mapped # Alternatives -We could allow additional dependent keys to be passed to filter/map in a -different way, such as via an options object: - -```js -foo: filter( - 'strings', - { - dependentKeys: ['filterText'] - }, - function(s) { - return s.includes(this.filterText); - } -) +We could allow additional dependent keys to be passed in as arbitrary arguments: -// with ember-decorators -@filter('strings', { - dependentKeys: ['filterText'] -}) -foo() { - return s.includes(this.filterText); -} +```ts +filter(...dependentKeys: string[], callback: Function): ComputedProperty; +map(...dependentKeys: string[], callback: Function): ComputedProperty; ``` + +This would potentially be confusing since the only key which would be used by +the macro directly would be the first key. From 127768d3b8531a9fb77cd524973e0d1e394acd18 Mon Sep 17 00:00:00 2001 From: Christopher Garrett Date: Thu, 27 Sep 2018 15:18:06 -0700 Subject: [PATCH 3/3] update function signatures --- ...00-deprecate-computed-property-modifier.md | 38 +++++++++++-------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/text/0000-deprecate-computed-property-modifier.md b/text/0000-deprecate-computed-property-modifier.md index cf998acfea..1b89e373b5 100644 --- a/text/0000-deprecate-computed-property-modifier.md +++ b/text/0000-deprecate-computed-property-modifier.md @@ -56,21 +56,23 @@ 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 configuration object mode. -The new API signatures would be: +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, options: { - callback: Function, - dependentKeys: string[] -}): ComputedProperty; +function filter( + filteredPropertyKey: string, + additionalDependentKeys: string[], + callback: Function +): ComputedProperty; function map(mappedPropertyKey: string, callback: Function): ComputedProperty; -function map(mappedPropertyKey: string, options: { - callback: Function, - dependentKeys: string[] -}): ComputedProperty; +function map( + mappedPropertyKey: string, + additionalDependentKeys: string[], + callback: Function +): ComputedProperty; ``` ## Deprecation Timeline @@ -104,12 +106,18 @@ only the first argument will be filtered/mapped # Alternatives -We could allow additional dependent keys to be passed in as arbitrary arguments: +We could allow additional dependent keys to be passed via an options argument: ```ts -filter(...dependentKeys: string[], callback: Function): ComputedProperty; -map(...dependentKeys: string[], callback: Function): ComputedProperty; +filter(dependentKey: string, options: { + additionalDependentKeys: [], + callback: Function, +}): ComputedProperty; + +map(dependentKey: string, options: { + additionalDependentKeys: [], + callback: Function, +}): ComputedProperty; ``` -This would potentially be confusing since the only key which would be used by -the macro directly would be the first key. +This is more verbose, but would be very clear.