Add just-order-by package

[klaseca]

Adding a orderBy function similar to orderBy from lodash

[angus-c]

Thanks Maxim! I like this but can we drop the field type param option. I think it is a little confusing and I believe it goes beyond what lodash does. Thanks.

[klaseca]

I do not quite understand. Are you suggesting to change OrderParam type?

// from
type OrderParam<T> = (
  | OrderParamField<T>
  | { field: OrderParamField<T>; order?: 'asc' | 'desc' }
);

// to
type OrderParam<T> = (
  | OrderParamField<T>
  | { order?: 'asc' | 'desc' }
);

[angus-c]

I might be wrong but it seems there only needs to be two possible argument formats (and I think this how lodash works too, although their documentation is not clear)

Sort definition is an array of keys, order is an array of strings
(arr: T[], sort: [keyof T, ...keyof T], order: ('asc' | 'desc')[])

Sort definition is a function, order is a string
(arr: T[], sort: (value: T) => Result, order: ('asc' | 'desc'))

[klaseca]

In fact, this is only a part of all possible argument formats (as far as I can see from their source code)

I purposely did not copy api from lodash. It seems more convenient to me to use an object as a parameter (it is immediately visible by which field and in what order sorting is performed)

Do you think the lodash api is better or is there some other reason to use it?

[angus-c]

Not wedded to lodash API

While I do like the idea of only having only two args, it just seemed a little confusing having an {field: <value>} for descending and function cases and <string> for ascending case.

I'm not really sure why we need an option for function type params at all. Either the function just points to the property name (don't need function), or if it's more complex it could just take the place of the entire sort function and user could use Array.prototype.sort directly

My personal preference is either:
orderBy(arr, [prop1, prop2], [order1, order2])
(prop1 could be a path to nested property a.b.c, third arg is optional and defaults to ascending for all props)
or
orderBy(arr, [{prop: prop1}, {prop: prop2, order: 'desc'}])
(order property is optional and defaults to 'asc', again value of prop can be path to nested prop)

[klaseca]

As I see it:

orderBy(arr, [
  (a) => a.b.c.toLowerCase(),
  'f',
  /* ^^^^^^^^^^^^^^^^^^^^^^^^
    Variant 1:
    Use an element as a function or a string, order is always asc
    String is name of top level field of object
    Inside function you can use nested fields of object and transform value
  */
  { field: 'd' },
  { field: 'e', order: 'desc' },
  { field: (a) => a.b['g.g'] },
  { field: (a) => a.b.h, order: 'desc' },
  /* ^^^^^^^^^^^^^^^^^^^^^^^^
    Variant 2:
    Use an element as object
    Field named 'field' is required and behavior is identical to Variant 1
    Field named 'order' is optional and defaults to asc
  */
]);

I think it is as simple as possible

As for string as a path to field: using a string not as a field name, but as a path to field is harder to implement, harder to declare types, and ultimately inferior to capabilities of function

[angus-c]

I don't think it should support function args. Array.prototype.sort is already suited for that approach IMO. I would prefer the AP I to be simple and have only one variant.

I like variant 2. We use path to field in https://github.com/angus-c/just#just-safe-get and it's not too difficult.

[klaseca]

I agree to leave only variant 2. But I do not agree with using string as path to field

Problems:

1. From guidelines:

Modules must not depend on any other npm modules (including other just modules)

This means that we will have to copy code from just-safe-get and not use it as a dependency. This is a stupid duplication of code and if just-safe-get is updated, we need to do same in this package (and this probably will not happen)

2. Type Declarations

The just-safe-get package is poorly typed. This can be fixed, but we go back to first problem (keeping the code up to date)

And as I wrote above, function still provides more features

Also as an example: just-sort-by package allows us to use a string (as name of top level field of object) or a function

[angus-c]

First of all my apologies. I completely forgot about the existence of just-sort-by. I rarely get time to work on Just these days, and so it slipped my mind.

In this case it makes perfect sense to follow a similar API. So yes let's keep functions, and no string to path. I would say for functional arg we should use your variant 1 style above (I don't think variant 2 is necessary for functional args since the function itself can specify the sort order—and I always want to have just one way to do things). I would prefer property to field for the key in variant 2, just because that is the conventional JS term. Or key is ok too if we want to shorten it.

A general note about occasional duplication of code in Just. It is not ideal but IMO it is better than the alternative. The purpose of Just is to make modules as tiny as possible so they a) do not cause a long network fetch b) do not bloat memory on older devices. Lodash modular version auto-inlines all dependencies into one module. It may be a little easier to maintain but it makes the JS much bigger since because it almost always inlines much more code than it actually needs. I would prefer to occasionally rewrite the same code in the interests of legibility and file size.

[klaseca]

The final api looks like this:

orderBy(arr, [
  { property: 'd' },
  { property: 'e', order: 'desc' },
  { property: (a) => a.b['g.g'] },
  { property: (a) => a.b.h, order: 'desc' },
]);

Correct me if something is wrong. If you agree, I will rewrite code as we agreed

[angus-c]

Looks good except I don't believe functional version needs order prop since this can be build into the function impl. However if you feel strongly then ok to keep it.

[angus-c]

Thank you for your patience.

Landing now.
For some reason the checks are failing (optional chaining complaint for node 12, timeouts for others)

I made a couple of changes in master (removed 12.x test, updated version in lerna config to match package.json version) and that worked locally. Would you mind rebasing from master to see if the checks now pass?

[angus-c]

@klaseca We're getting 2 test errors
https://github.com/angus-c/just/actions/runs/4397574303/jobs/7962546964?pr=530#step:5:305

[klaseca]

Fixed

[angus-c]

Thanks! Published as just-order-by@1.0.0
REPL here https://anguscroll.com/just/just-order-by (may not show up immediately)