feat: Support sorting by multiple values. #19
Conversation
| @@ -39,7 +39,7 @@ export function isDefined<T extends any>(param: T | undefined | null): param is | |||
| return param !== null && param !== undefined; | |||
| } | |||
|
|
|||
| export type Comparable = string | number | bigint | Date | Temporal.PlainDate | Temporal.ZonedDateTime; | |||
| export type Comparable = string | number | bigint | Date | Temporal.PlainDate | Temporal.ZonedDateTime | undefined; | |||
There was a problem hiding this comment.
We check for undefined on the 1st line of compare's impl, so seems like undefined was already allowed as a Comparable
| const av = fn(a); | ||
| const bv = fn(b); | ||
| if (Array.isArray(av) && Array.isArray(bv)) { | ||
| for (let i = 0; i < av.length; i++) { |
There was a problem hiding this comment.
issue: i think we need to consider if the arrays have different lengths? eg if bv was [1,2,3] and av was [1,2], then bv should sort before, but with this code they'd sort as equal.
There was a problem hiding this comment.
I was thinking of finding some fancy TypeScript that would force the lambda to always return the same length-d array; ChatGTP had some suggestions:
type LambdaWithFixedLength<N extends number, T = unknown> = (...args: any[]) => FixedLengthArray<T, N>;
type FixedLengthArray<T, N extends number, R extends unknown[] = []> = R["length"] extends N
? R
: FixedLengthArray<T, N, [T, ...R]>;But I couldn't get it to work in the time box I had for adding this, so was pretty tempted to be satisfied with the "don't do that" version of the lambda returning different length-d arrays.
I suppose could do a runtime error if they're different lengths. Pushed that up.
Does that sgty?
There was a problem hiding this comment.
I dunno i feel like different length arrays is a valid use case and not that hard to support? eg, lets say you were doing semantic versioning and to sort you emitted the version as an array of numbers. The arrays might have different lengths but they'd still be meaningful.
There was a problem hiding this comment.
🤔 maybe I guess; personally I'd assume that different length arrays in the wild would 80% of the time be bugs the programmer didn't know/mean to write. 😢
There was a problem hiding this comment.
i think you're just envisioning the use case where you'd have like foos.sortBy((foo) => [foo.bar, foo.baz]) and i'm envisioning something more dynamic? I don't think the dynamic case is that unrealistic to use. Also it'd be like a 1 line change:
| for (let i = 0; i < av.length; i++) { | |
| const length = Math.max(av.length, bv.length); | |
| for (let i = 0; i < length; i++) { |
No description provided.