Ember.computed.sortBy

[workmanw]

Rendered

It's not really a big RFC. Happy to add any further detail, I just didn't want to draw it out when it was straight forward.

[opsb]

This would be nice. The common case is that you specify the ordering explicitly, it's rare that I've wanted to bind the sort properties themselves.

[rlivsey]

👍 it's extremely rare that I update the sort property so making the default be simpler is great.

I wonder if you could detect if it's a sort definition by the presence of ':' or whether that would be too confusing, so you could use a string instead of an array:

newestPosts: Ember.computed.sort('posts', 'createdOn:desc')

That might be too easy to footgun yourself by leaving out the sort order and wonder why it doesn't work:

newestPosts: Ember.computed.sort('posts', 'createdOn')

[opsb]

How about introducing Ember.computed.sortBy?

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'])

edit: added multiple sort properties example

[workmanw]

@opsb Yea, I had mentioned this in my alternatives, but my Ember.computed.readOnlySort was a far less attractive name. I'll update my RFC.

[mitchlloyd]

You've been writing too much Python ;)

=> //

[workmanw]

Hahaha. My brain didn't even notice. :)

[opsb]

Ha, it's a ruby week for me...

[mitchlloyd]

To me the current alternative looks a little better than the proposal. It lines up nicely with the Ember array methods.

This seems like a useful feature and a nice API. In the past people have been adverse to adding more CPMs to Ember, encouraging user-land libraries like https://github.com/cibernox/ember-cpm instead. It will be interesting to hear more feedback.

[workmanw]

@mitchlloyd The more I thought about it, the more that I do like the alternative. But I think we [ember] tend to add lots of little things, and then fold them back together. My inclination was to not increase the surface of the API.

But yea, we'll see how interested in this people actually are. If it gets a luke warm response, I'll open a pull to ember-cpm. :)

[MiguelMadero]

minor: this doesn't need naturally, I would expect it to read sortBy fieldNameImSorting e.g. sortBy firstName. This is the case for array.sortBy and other similar ones like filterBy and mapBy. When we have the arrayPropertyName as the first parameter it breaks the readability. I think that extending the current API makes more sense.

[mitchlloyd]

For those of us who lean towards a more functional style when they can, this actually reads pretty normally. For instance with LoDash this would be sortBy(posts, 'createdOn'). Also see computed.filterBy.

[MiguelMadero]

sounds good

[lukemelia]

I'm in favor of introducing Ember.computed.sortBy. I've long found Ember.computed.sort to have a confusing API, and I think that leaving it as is for advanced scenarios, and having sortBy for simpler ones is a big usability win.

[stefanpenner]

I like the idea of a sort by

[workmanw]

After further consideration, I too think sortBy is better. And it seems that's pretty much the consensus here. I'm looking for advice on how to transition this RFC to reflect that. Should I modify this RFC to reflect that, should I close this and open a new one, or should just leave this as is for now?

Also, FWIW, if there is a green light for sortBy, I'd be happy to implement it. I have some familiarity with the implementation of Ember.computed.sort.

[mitchlloyd]

@workmanw I've noticed that some RFC PRs here change dramatically before they are merged. For instance the approach in the "contextual components" RFC changed so much that the name of the branch (contextual-component-lookup) is at odds with where the proposal landed. Seems like it's nice to keep the history together even if the branch name isn't perfect.

[workmanw]

@mitchlloyd Thanks!

I've updated this RFC to reflect Ember.computed.sortBy as the proposal.

[rwjblue]

I am 👍, I'll try to bring this up at the next core team meeting. In the meantime, can you create an addon that provides this capability (which could be used as a polyfill once this feature lands)?

[workmanw]

@rwjblue Sure thing, I'd be happy to get moving on it this weekend. Ultimately, I may have wait for emberjs/ember.js#12221 to be resolved (also see: emberjs/ember.js#12212). But I'll definitely see how far I can get.

EDIT: Actually I think because the sort order is immutable, I could make this work without emberjs/ember.js#12221. Stay tuned.

[workmanw]

@rwjblue Done!
I created workmanw/ember-computed-sortby and published it to npm. I included basic docs in the README and code. And tests of course.

For those interested in trying it you can use: ember install ember-computed-sortby.

---

Edit: This is my first actual public addon. If I messed something up or forgot something, please let me know.

[mixonic]

@workmanw could this just be an update to sort to accept an array as the second argument?

Or alternatively if you suggest sortBy('posts', 'createdOn:desc'), why keep the array syntax at all? Just allow multiple arguments after the first.

[workmanw]

@mixonic

Could this just be an update to sort to accept an array as the second argument?

This was actually the initial suggestion of this RFC. Ember.computed.sortBy was the alternative. But there was consensus that Ember.computed.sortBy was more favorable. After further consideration, I started to agree. My primary reason was that having sortBy as a separate macro meant you could provide a nicer API and made it easier for someone reading the code to know exactly what it was doing.

Or alternatively if you suggest sortBy('posts', 'createdOn:desc'), why keep the array syntax at all? Just allow multiple arguments after the first.

This is a pretty good idea that hadn't occurred to me. It also matches the API of Ember.Enumerable.sortBy. I think this is probably worth adopting.

[mixonic]

Nice. My other lingering thought, though I don't know where to go with it, is that microsyntaxes ala prop:desc generally become troublesome later on. Some wacky ideas:

sorted: Ember.computed.sortBy('thing', 'propA', ['propB', 'desc'], ['propC']);
sorted: Ember.computed.sortBy('thing', { property: 'propA' }, { property: 'propB', direction: 'desc' });

We don't need to avoid the microsyntax, but I encourage you to explore some other ideas before we lock it in.

[workmanw]

I updated the RFC and the polyfill repo to remove the array syntax for the sortDefinitions.

@mixonic I've given some thought to alternatives to the microsyntax, but I haven't come up with anything I loved. Everything just felt too verbose. I'm still open to exploring this, but given that Ember.computed.sort still relies on ':desc', I'm inclined to not break with the pattern.

FWIW, the best alternate idea I came up with was:

sorted: Ember.computed.sortBy('thing', 'propA', 'propB', false, 'propC');
sorted: Ember.computed.sortBy('thing', 'propA', 'propB', -1, 'propC');

I had also thought about making a symbol for desc that could be specific to Ember. Something like:

Ember.descending = Symbol('desc');
sorted: Ember.computed.sortBy('thing', 'propA', 'propB', Ember.descending, 'propC');

But I loved this even less.

[mitchlloyd]

I don't mind the tuple style arrays. They are still pretty concise without inventing syntax in a string.

Ember.computed.sortBy('thing', 'propA', ['propB', 'desc'], ['propC', 'asc']);

[lukemelia]

I find the microsyntax easier to read and I'd posit it is just as likely to be future-friendly as a tuple-style array.

[rwjblue]

To throw my hat in the ring:

I prefer the easy to understand but slightly more verbose versions:

// option 1
Ember.computed.sortBy('thing', 'propA', {propB: 'desc', propC: 'asc'});

// option 2
Ember.computed.sortBy('thing', 'propA', {property: 'propB', direction: 'desc'}, { property: 'propC', direction: 'asc'});

I like "option 1" the best of these two.

I am perfectly happy with supporting the microsyntax also, but would prefer for it to just be sugar on top of a better/easier to grok thing. I find that these various microsyntaxes (that look similar but are different) are quite often a barrier to entry for new folks, and "Option 1" above is actually pretty close to the same number of total characters (slightly more, but not by much)...

[kimroen]

I agree that considering Ember.computed.sort uses :desc I'd expect that to work here too. Then again, sortBy elsewhere does not have this syntax, which might be confusing. No matter what we come up with here, it's something we made up for this purpose, so I'm not sure it matters either way - anything would be experienced as different.

---

@rwjblue In your option 1, would these have the same behavior?

Ember.computed.sortBy('thing', 'propA', {propB: 'desc', propC: 'asc'});
Ember.computed.sortBy('thing', 'propA', {propB: 'desc'}, 'propC');
Ember.computed.sortBy('thing', 'propA', {propB: 'desc'}, {propC: 'asc'});

Or to put it differently; what are the rules for the objects? Are they actually easier to understand?

[workmanw]

I would say that I personally still lean toward the microsyntax because I find it the most concise and readable. Out of the ones proposed I liked this one the best:

Ember.computed.sortBy('thing', 'propA', ['propB', 'desc'], ['propC', 'asc']);

It feels like the easiest to read. The two alternates with pojos felt a little more jarring, especially with syntax highlighting. All that said, I don't have a strong opinion about the format at all. I'd be happy with anything that's been suggested here.

[mitchlloyd]

One more variation on the options from @rwjblue.

Ember.computed.sortBy('thing', 'propA', { propB: 'desc' }, { propC: 'asc' });

Having the constraint of one object per property might make things more consistent given what @kimroen has shown. "micro:syntax", tuples, objects, no strong preference here either.

[opsb]

As this is an exercise in ergonomics the microsyntax seems like the best option. It matches the existing syntax for Ember.computed.sort, it just provides a shortcut so that you don't have to use a separate property to specify the sort config.

[rwjblue]

@kimroen - It was late and I mistyped (or copied), I meant to take two arguments the array and an object. My example should have been:

Ember.computed.sortBy('thing', { propA: 'asc', propB: 'desc', propC: 'asc' });

Also, the ergonomics here (IMHO) should not be about fewest characters typed, but readability/ease of understanding. We can always start with something like I have listed above (which is still much better than having a second property that you have to go track down), and add a super slim string based microsyntax that desugars into the above later...

[opsb]

I suppose the reason I'm drawn to the microsyntax is that I've already been using it with ember for years, by now it is the most readable syntax. Newcomers may have a different opinion though. I don't see any issue with supporting both syntaxes but I think it would be nice to include the microsyntax for those of us that already familiar with it.

[mixonic]

As I began the bikeshedding, I will now put an end to it :-)

The place where the microsyntax is painful is not this proposed sortBy, but sort. sort is often used for headers that can change how the content is ordered when clicked, and in this case mucking with strings is unfortunate.

As sortBy is static, the microsyntax seems harmless. I support using it for sortBy. If anyone else feels strongly otherwise they should make a case advocating for their position. Thanks for the exploration folks!

[workmanw]

@mixonic Alrighty :)

I'm happy to continue spending cycles where ever necessary to keep this RFC moving forward. With respect to the microsyntax, it seems like pretty split presently. It may just be one of those things the core team has to call the ball on.

[workmanw]

It's been a few months since there has been any action on this. I have an addon that we've been using for the past few months in production. I currently have the bandwidth to submit an Ember PR if it seems valuable for this to reside in the core. If not, that's fine too, we can close this and keep it as an addon.

I'm not sure what the next step here is. The contributing.md is a little vague, but it sounds like I should just submit a PR? It was mentioned bringing this up at a core team meeting for discussion. So I'm not really sure what the next steps are or if the ball is even in my court.

[mixonic]

@workmanw thanks for bringing it up. I will put this on the next core agenda for approval. Is the addon public? Can you link me up?

Sorry for the delay, but I will try to get you unblocked on Friday.

[workmanw]

@mixonic Yeap! Here you go: https://github.com/workmanw/ember-computed-sortby

[mmun]

Not really a fan of

Ember.computed.sortBy('thing', { propA: 'asc', propB: 'desc', propC: 'asc' });

as it does not express clearly the fact that the order of the sort properties matters.

I don't love the microsyntax API but we should implement it because it is what people expect. If we can decide on a bette API later, we can uniformly deprecate the microsyntax API everywhere that it is used and transition to the new API.

[workmanw]

Ping @mixonic :)

I don't mean to keep bugging you, but I'm in a holding pattern right now ✈️. I can just submit a PR if that would be a better place for this discussion to occur.

[mixonic]

@workmanw core discussions have been waylaid by the holidays :-/ There is strong push-back regarding adding any new CP macros. See also: emberjs/ember.js#12758

I will attempt to raise it again on Friday. Agreed that we should give you a solid direction.

[workmanw]

core discussions have been waylaid by the holidays.

@mixonic Yea that's what I figured, no worries :)

There is strong push-back regarding adding any new CP macros. See also: emberjs/ember.js#12758

Oh interesting. I must say that I certainly understand wanting to keep the surface of the API as small as possible and if sortBy isn't right for the core that's fine by me. That said, from @stefanpenner's comment in the linked issue, it sounds like that moratorium has been in place for a while? But no one, including the core team members who have commented here, thought to mention it. That would be :(.

[mixonic]

@workmanw! I owe you a direction, for better or worse.

We discussed this RFC and the general "CP moratorium" at today's meeting. The consensus is that we want to strictly limit introducing new macros, though there are some cases such as the addition of uniqBy to make our APIs consistent that are justified.

Additionally, there is a strong resistance to adding new microsyntax to Ember.

These two ideas (no new macros, no new microsyntax) mean we're going to close this RFC without accepting it. We are quite supportive of the ember-computed-sortby addon though. Thanks for the effort on this, and I hope you feel the process made for a better addon if not for an addition to Ember itself.

[workmanw]

Well I can't say I'm not a little disappointed, but I do appreciate it reaching a final conclusion. Thank you.

[MiguelMadero]

Thanks @workmanw @mixonic and everyone involved. I really appreciate the process, even when the outcome might seem negative, it sets a direction and precedent and there's good reasoning behind not doing it. Also, having the addon ecosystem enables innovation outside of core, which I think is perfect for this case.

[mmun]

I agree with the outcome of this RFC but I am still strongly in favor of filling in this hole in the API.

There are three sorting methods and they should all share a uniform API

• Ember.computed.sort
• Ember.computed.sortBy
• Ember.Enumerable#sortBy

Presently,

• Ember.Enumerable#sortBy can take a list of paths to sort by, but does not support the asc/desc microsyntax.
• Ember.computed.sort can take a list of paths to sort by and does support asc/desc microsyntax.
• Ember.computed.sortBy does not exist in core.

The path forward is thus,

• Deprecate the current the microsyntax used by Ember.computed.sort.
• Find a new way to express asc/desc without using a microsyntax. It must be backwards compatible with the microsyntax.
• Ensure that all three APIs uniformly use the new API.