feat: orderBy in the query builder

[samwillis]

the result collections .toArray is ordered by the order by, and there is a _orderByIndex prop on each result.

This also updates the D2TS dep to a new version that fixes an issue in compaction that was apparent when doing an order by. This may also fix the issue @KyleAMathews saw with joins, as I think that was the same thing happening.

[changeset-bot]

⚠️ No Changeset found

Latest commit: bf2f007

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[KyleAMathews]

This looks fantastic!

One small change though is could we do sortBy instead of sortFn?

Maybe this is from too many years of using lodash haha. https://lodash.com/docs/#sortBy

I also don't tend to love variable names with postfixes for their type as long as we can make it obvious by its name which sortBy does.

[samwillis]

Done, good call (I think I had mutationFn in my head from before).

I don't really expect people to use the sortBy option on the collection config, it's mostly an internal detail when constructing a collection from a query that has an orderBy.

[KyleAMathews]

Hmm though it's also confusing to have a config named sortBy and the SQL operator is orderBy 😂

Maybe comparator then as that's a typical name

[KyleAMathews]

Yeah it's just for custom sorts right? Normally you wouldn't need it?

[samwillis]

The query engine adds a (currently named) _orderByIndex to the results, if you use a limit+offset these are reflected in this value.

When we construct the derivedArray in the result collection, we want it to be sorted by this value - the sortBy/sortFn/sortComparitor is a mostly internal prop that enables you to add a callback that is then used on the derivedArray to sort it. A user could use this on a root collection if they wanted to, I suppose? that way it is sorted when synced into.

I think any of the three names would work.

Aside: there is some internal tooling inherited from D2QL that would let the user name the _orderByIndex differently, but it needs fleshing out and a decision on what the dx would be for that - something for later

[KyleAMathews]

Hmm so it's just a final sort on the sort already fine by d2ts? 🤔

Why make it an option at all then?

[samwillis]

The result collection could be sorted or not. It needs some way to know if it needs to sort the collection that has been returned. This could be a sorted flag, but then we have to make in the sorted value key on the result objects. Or it could look for the _orderByIndex on the results to infer it's sorted, but again that bakes in the prop used to sort.

I went with sortFn as it seemed extendable, and no tied to a particular implementation for the query engine.

Bare in mind that the output of the D2 pipeline is a series of changes, not a materialised list, so the internal sorted structure isn't accessible, thats why we annotate the results with the _orderByIndex.

I agree, it seems confusing that the sort is potentially done twice, but with offset (and limit) the sort needs to be done in the query pipeline. When we add CTEs/subqueries, which are sort of mostly done already, the order-by needs to in the pipeline, rather than after it.

There is a lot of opportunity to refine and optimise all this - but I think getting the UX right first, then finding the optimisations later, makes the most sense.

[KyleAMathews]

Can we completely keep the implementation internal? E.g. just the _orderByIndex? If we can and don't have any use cases for a user defined sort, that'd be my preference.

[samwillis]

Yep, probably the easiest way is to look at the first item in the collection, and if it has an _orderByIndex do the sort.

[samwillis]

@KyleAMathews I've changed this to use just infer that a collection should be sorted by looking for the _orderByIndex prop.

[samwillis]

The two errors in CI are the package reply due to the rename, and a formatting issue in the README - the latter is fixed in #66

[KyleAMathews]