radashi-org / radashi

The modern, community-first TypeScript toolkit with all of the fast, readable, and minimal utility functions you need. Type-safe, dependency-free, tree-shakeable, fully tested.
https://radashi.js.org
MIT License
315 stars 25 forks source link

`group()` returns optional object values #287

Closed aeharding closed 3 weeks ago

aeharding commented 3 weeks ago

When I call group, it returns the object value type as T | undefined. However, this is not correct (and differs from other implementations, like lodash.groupBy).

Here is an example:

import * as _ from 'radashi'

const returnValues = _.group([1, 1.2, 1.3, 2], Math.floor);
const sum = Object.values(returnValues).reduce(
  (acc, curr) => acc + curr.length,
  0,
);

console.log(sum);
image
aleclarson commented 3 weeks ago

TypeScript cannot know at compile-time what values are in the array passed to group, so it cannot guarantee the existence of any particular group.

That said, using exactOptionalPropertyTypes will be a valid workaround if https://github.com/microsoft/TypeScript/issues/46969 is ever fixed. I've added type test that will track this: 28c6efc

aeharding commented 3 weeks ago

OK thanks, however I don't understand why it is not possible since lodash has the following signature and it works fine. Why not use that instead?

groupBy<T>(collection: List<T> | null | undefined, iteratee?: ValueIteratee<T>): Dictionary<T[]>;

With lodash, the following errors ('value' is possibly 'null'.ts(18047)) because null is in the array:

import * as _ from "lodash";

const returnValues = _.groupBy([null, 1.2, 1.3, 2]);
const sum = Object.values(returnValues).forEach((group) => {
  group.forEach((value) => {
    console.log(value + 1);
  });
});

but if I remove null it works:

import * as _ from "lodash";

const returnValues = _.groupBy([1.2, 1.3, 2]);
const sum = Object.values(returnValues).forEach((group) => {
  group.forEach((value) => {
    console.log(value + 1);
  });
});
aleclarson commented 3 weeks ago

Lodash's solution is only type-safe if you're using Object.values (or similar) on the result. If you're accessing the groups via property access, it will lie to you about the type.

const { even, odd } = _.group([1, 3, 5], n => isEven(n) ? 'even' : 'odd')
// In Lodash, `even` is `number[]` when it will actually be undefined at runtime

A better solution is to extend our cluster function to accept an optional "identity function" for cases where all you're doing is calling Object.values immediately after.

Example

const result = cluster([null, 1.2, 1.3, 2], Math.floor)
//    ^? (number | null)[][]

result
// => [[null], [1.2, 1.3], [2]]
aeharding commented 3 weeks ago

Oh, I see! 🤦‍♂️ Thanks for the explanation. Hopefully it will help someone else running into the same issue migrating from lodash.