Added new enum-based sortBy query argument

[Vultraz]

Resolves #183

Used sortBy as suggested by @molomby. I also made it so you can specify multiple sort options for multi-field/column sorting (which both MongoDB and Knex support).

allUsers(sortBy: [name_ASC, isAdmin_DESC]) {
  name
}

for example.

The single-criteria sort also works:

allUsers(sortBy: name_ASC) {
  name
}

Generated enum example:

@timleslie

[changeset-bot]

🦋 Changeset is good to go

Latest commit: 0da88ac

We got this.

This PR includes changesets to release 4 packages

Name	Type
@keystonejs/adapter-knex	Minor
@keystonejs/adapter-mongoose	Minor
@keystonejs/keystone	Minor
@keystonejs/mongo-join-builder	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[molomby]

Oh this is great 🎉 I've been wanting to fix this issue for ages. Thanks @Vultraz 🙌🏼

[Vultraz]

...heh. Seems Node 10 doesn't have Object.fromEntries.

[Vultraz]

Reduced changeset level to minor since this doesn't actually break orderBy.

[Vultraz]

Might need some work to make the Admin UI use this, but that could be done separately.

[timleslie]

I really like this, and it's definitely something I want to pull in. One challenge that I want to think through before we merge it (because it will be hard to change later) is the naming.

I had this as an IRL conversation with @molomby , but I don't like the use of the term sort here, because in some sense it implies (to me) that the underlying data is being sorted, whereas order (to me) is more related to how the data is being returned from the query.

This is something I need to think through a bit more deeply and work out a solution for (which might just require me to change my brain).

[Vultraz]

Hmm 🤔 IMO, since either order or sort is only a query argument, it conceptually makes sense that either would apply only to the returned result. If it was also a mutation argument, then one might think it was also sorting the DB data.

[molomby]

@timleslie, sure you're not getting that backwards? When we spoke in Jan I'm pretty sure you agreed sortBy was better in this context and should replace orderBy.

The reasoning was that "order" implies the list itself has a natural or explicit ordering stored within it. Eg. a list with a displayOrder int or the "ordered" list type we were talking about.

A "sort" on the other hand, is something you apply to one or more arbitrary keys in a more dynamic fashion. Eg. I'm going to sort this set of buildings by city then height.

Similar to the vibe portrayed here.

[molomby]

I'm really keen to get this merged but have to ask... should the sortBy tests be replacing the orderBy tests or should we keep both? Even with orderBy being deprecated, it'd be nice to not break it.

[molomby]

Personally, I think this is correct. Should get sign off from @timleslie too though before merging.

[timleslie]

Righto, I think this is good to go. Let's 🚢 this thing and see what happens!

Thanks for the implementation @Vultraz and for your patience while we got around to fully reviewing it 👍

[timleslie]

Let's 🚢

[timleslie]

Righto, I think this is good to go. Let's 🚢 this thing and see what happens!

Thanks for the implementation @Vultraz and for your patience while we got around to fully reviewing it 👍

[Vultraz]

No problem! 😃

@Nikodermus I think you were waiting for this.