From 165f4e22dd296409de4da4e22304fa17eebc9d6b Mon Sep 17 00:00:00 2001 From: Wesley Workman Date: Wed, 26 Aug 2015 08:36:25 -0400 Subject: [PATCH 1/4] RFC to make Ember.computed.sort more convenient for some use cases. --- active/0000-computed-sort-array-argument.md | 56 +++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 active/0000-computed-sort-array-argument.md diff --git a/active/0000-computed-sort-array-argument.md b/active/0000-computed-sort-array-argument.md new file mode 100644 index 0000000000..e91f31686e --- /dev/null +++ b/active/0000-computed-sort-array-argument.md @@ -0,0 +1,56 @@ +- Start Date: 2015-08-26 +- RFC PR: (leave this empty) +- Ember Issue: (leave this empty) + +# Summary + +In Ember 2.x, `Ember.computed.sort` is the one and only computed macro for sorting a list. Its current API is built assuming that the sort order may change at runtime, leaving no way to easily create a declarative sort macro that you know is immutable. This RFC proposes adding an additional argument type for the `sortDefinition` parameter to make it easier to declare and to ensure sort order is immutable. + +# Motivation + +I'm hoping to satisfy two primary annoyances with this RFC. First, `Ember.computed.sort` can be used as a singular definition as the sort order will be the value of the second argument, you will not have to look for another definition on the class, thus improving code readability. In our app, we often have the following: + +```javascript +var NewestPostsController = Ember.Controller.extend({ + posts: null, + newestPosts: Ember.computed.sort('posts', '_newestPostsOrder'), + _newestPostsOrder: ['createdOn:desc'] +}); +``` + +The second annoyance is I know that if `_newestPostsOrder` was inadvertently changed, it could cause `newestPosts` to be incorrect. I realize this would just be a bug in our app, but having the ability to define an immutable sort order would give the same peace of mind as a readOnly computed property. + +# Detailed design + +Right now the second argument for `Ember.computed.sort`, `sortDefinition`, accepts two values, a string and a function. I'm proposing we add array as a third allowed argument type. The array would expect the exact same sort properties format as it does when using a string as a dependent key. Reusing the above example, + +Before: +```javascript +var NewestPostsController = Ember.Controller.extend({ + posts: null, + newestPosts: Ember.computed.sort('posts', '_newestPostsOrder'), + _newestPostsOrder: ['createdOn:desc'] +}); +``` + +After: +```javascript +var NewestPostsController = Ember.Controller.extend({ + posts: null, + newestPosts: Ember.computed.sort('posts', ['createdOn:desc']) +}); +``` + +This design would not break semver as it's an additional argument type that was previously not supported. + +# Drawbacks + +I can think of two drawbacks. First and foremost, the array argument type will become reserved for this functionality. It could not easily be reused for a new extension in the future. Second, it's ultimately just a more convenient way of doing something that is already functionally possible. + +# Alternatives + +No real alternatives were considered. The only thing that came to mind as an alternative was adding an additional computed macro for this use case (EG `Ember.computed.readOnlySort`). + +# Unresolved questions + +I don't believe there are any unresolved questions at this time. From bfa15324ee5e613e274fd7dad228465a6169f1cf Mon Sep 17 00:00:00 2001 From: Wesley Workman Date: Wed, 26 Aug 2015 15:57:29 -0400 Subject: [PATCH 2/4] Update computed-sort-array RFC to add alternative suggested by @opsb. --- active/0000-computed-sort-array-argument.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/active/0000-computed-sort-array-argument.md b/active/0000-computed-sort-array-argument.md index e91f31686e..08da235488 100644 --- a/active/0000-computed-sort-array-argument.md +++ b/active/0000-computed-sort-array-argument.md @@ -49,7 +49,17 @@ I can think of two drawbacks. First and foremost, the array argument type will b # Alternatives -No real alternatives were considered. The only thing that came to mind as an alternative was adding an additional computed macro for this use case (EG `Ember.computed.readOnlySort`). +An alternative would be to implement a new computed macro called: `Ember.computed.sortBy`. This would allow for more argument/API flexibility. Here a few examples: + +```javascript +newestPosts: Ember.computed.sortBy('posts', 'createdOn') # default to ascending + +newestPosts: Ember.computed.sortBy('posts', 'createdOn:desc') + +newestPosts: Ember.computed.sortBy('posts', ['author.name', 'createdOn:desc']) +``` + +Credit for this alternative goes to Oliver Searle-Barnes (@opsb). # Unresolved questions From 6c2268e6649d997a2632b98c6ac2f0da2d12a269 Mon Sep 17 00:00:00 2001 From: Wesley Workman Date: Wed, 9 Sep 2015 13:40:19 -0400 Subject: [PATCH 3/4] Updated my RFC to reflect the new direction of using Ember.computed.sortBy. --- active/0000-computed-sort-array-argument.md | 32 +++++++++------------ 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/active/0000-computed-sort-array-argument.md b/active/0000-computed-sort-array-argument.md index 08da235488..9d91441f37 100644 --- a/active/0000-computed-sort-array-argument.md +++ b/active/0000-computed-sort-array-argument.md @@ -4,17 +4,17 @@ # Summary -In Ember 2.x, `Ember.computed.sort` is the one and only computed macro for sorting a list. Its current API is built assuming that the sort order may change at runtime, leaving no way to easily create a declarative sort macro that you know is immutable. This RFC proposes adding an additional argument type for the `sortDefinition` parameter to make it easier to declare and to ensure sort order is immutable. +In Ember 2.x, `Ember.computed.sort` is the one and only computed macro for sorting a list. Its current API is built assuming that the sort order may change at runtime, leaving no way to easily create a declarative sort macro that you know is immutable. This RFC proposes adding an additional computed macro for declarative and immutable sorting. # Motivation -I'm hoping to satisfy two primary annoyances with this RFC. First, `Ember.computed.sort` can be used as a singular definition as the sort order will be the value of the second argument, you will not have to look for another definition on the class, thus improving code readability. In our app, we often have the following: +I'm hoping to satisfy two primary annoyances with this RFC. First, providing a way to declare a computed sort macro as a singular definition (one line of code). As opposed to `Ember.computed.sort` which requires two declarations to use. It's my belief this will improve readability for some use cases. In our app, we often have the following: ```javascript var NewestPostsController = Ember.Controller.extend({ posts: null, newestPosts: Ember.computed.sort('posts', '_newestPostsOrder'), - _newestPostsOrder: ['createdOn:desc'] + _newestPostsOrder: ['createdOn:desc', 'upvoteCount'] }); ``` @@ -22,14 +22,14 @@ The second annoyance is I know that if `_newestPostsOrder` was inadvertently cha # Detailed design -Right now the second argument for `Ember.computed.sort`, `sortDefinition`, accepts two values, a string and a function. I'm proposing we add array as a third allowed argument type. The array would expect the exact same sort properties format as it does when using a string as a dependent key. Reusing the above example, +With this RFC I'm proposing we introduce an additional computed macro, `Ember.computed.sortBy`, to fill the gap. The argument order would be similar to that of `Ember.computed.sort`. Reusing the above example, Before: ```javascript var NewestPostsController = Ember.Controller.extend({ posts: null, newestPosts: Ember.computed.sort('posts', '_newestPostsOrder'), - _newestPostsOrder: ['createdOn:desc'] + _newestPostsOrder: ['createdOn:desc', 'upvoteCount'] }); ``` @@ -37,29 +37,23 @@ After: ```javascript var NewestPostsController = Ember.Controller.extend({ posts: null, - newestPosts: Ember.computed.sort('posts', ['createdOn:desc']) + newestPosts: Ember.computed.sortBy('posts', ['createdOn:desc', 'upvoteCount']) }); ``` -This design would not break semver as it's an additional argument type that was previously not supported. +Additionally, the sort order could be defined as a string instead of a single item array. Eg: +```javascript + newestPosts: Ember.computed.sortBy('posts', 'createdOn:desc') +``` + # Drawbacks -I can think of two drawbacks. First and foremost, the array argument type will become reserved for this functionality. It could not easily be reused for a new extension in the future. Second, it's ultimately just a more convenient way of doing something that is already functionally possible. +The only draw back I could come up with is that this increases the surface of the API and is ultimately just a more convenient way of doing something that is already functionally possible. # Alternatives -An alternative would be to implement a new computed macro called: `Ember.computed.sortBy`. This would allow for more argument/API flexibility. Here a few examples: - -```javascript -newestPosts: Ember.computed.sortBy('posts', 'createdOn') # default to ascending - -newestPosts: Ember.computed.sortBy('posts', 'createdOn:desc') - -newestPosts: Ember.computed.sortBy('posts', ['author.name', 'createdOn:desc']) -``` - -Credit for this alternative goes to Oliver Searle-Barnes (@opsb). +When I originally drafted this RFC, `Ember.computed.sortBy` was the alternative, but it's became the clear favorite. So flipping this on it's head, the alternative here is modify the existing `Ember.computed.sort` to have this behavior. Additionally, this could be implemented as an addon or perhaps added to [ember-cpm](https://github.com/cibernox/ember-cpm). # Unresolved questions From 053fdf390c67bd2dab7a3b135f71595736761c94 Mon Sep 17 00:00:00 2001 From: Wesley Workman Date: Mon, 14 Sep 2015 08:08:37 -0400 Subject: [PATCH 4/4] Updated the RFC to reflect the new argument structure suggested by @mixonic. --- active/0000-computed-sort-array-argument.md | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/active/0000-computed-sort-array-argument.md b/active/0000-computed-sort-array-argument.md index 9d91441f37..e1efac9375 100644 --- a/active/0000-computed-sort-array-argument.md +++ b/active/0000-computed-sort-array-argument.md @@ -22,7 +22,7 @@ The second annoyance is I know that if `_newestPostsOrder` was inadvertently cha # Detailed design -With this RFC I'm proposing we introduce an additional computed macro, `Ember.computed.sortBy`, to fill the gap. The argument order would be similar to that of `Ember.computed.sort`. Reusing the above example, +With this RFC I'm proposing we introduce an additional computed macro, `Ember.computed.sortBy`, to fill the gap. The argument style would be similar to that of `Ember.computed.sort`. However, with `Ember.computed.sortBy` all non-primary sort orders (secondary, tertiary, etc) would be defined as additional arguments. `Ember.Enumerable.sortBy` serves as existing precedence for this pattern. Reusing the above example, Before: ```javascript @@ -37,15 +37,10 @@ After: ```javascript var NewestPostsController = Ember.Controller.extend({ posts: null, - newestPosts: Ember.computed.sortBy('posts', ['createdOn:desc', 'upvoteCount']) + newestPosts: Ember.computed.sortBy('posts', 'createdOn:desc', 'upvoteCount') }); ``` -Additionally, the sort order could be defined as a string instead of a single item array. Eg: -```javascript - newestPosts: Ember.computed.sortBy('posts', 'createdOn:desc') -``` - # Drawbacks