Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: fix(groupBy) do not return wrapped in Partial<> #94

Closed
wants to merge 3 commits into from

Conversation

Harris-Miller
Copy link
Collaborator

A previous MR #45 added to the return of groupBy that it should be wrapped in Partial<>. This makes a lot of sense in the fact that the function could return keys that don't actually exist on the object it creates.

Using the example from Ramda docs

const byGrade = groupBy((student: { score: number; name: string }) => {
  const score = student.score;
  return score < 65 ? 'F' : score < 70 ? 'D' : score < 80 ? 'C' : score < 90 ? 'B' : 'A';
});
const students = [
  { name: 'Abby', score: 84 },
  { name: 'Eddy', score: 58 },
  { name: 'Jack', score: 69 }
];

The type that byGrade(students) returned previously was Record<'A' | 'B' | 'C' | 'D' | 'F', Student[]>, which was accurate, but also misleading, because there was no guarantee at runtime that each of the union values existed as a key. This is why the Partial<> as added.

6 months later

HIndsight is 20/20 and we've come to learn that wrapping in Partial<> breaks other functionality in terms of mapping to other things. toPairs is a prime example.

const entries = toPairs(groupBy(students));

The return type previously was [<'A' | 'B' | 'C' | 'D' | 'F', Student[] | undefined][]. The | undefined is a problem. Because that it more inaccurate for runtime behavior that index accessing on Record<string, Student[]>

Typescript also allows for what we were trying to solve here via the tsconfig option https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess. Which if set gives a project a correct Students[] | undefined from index accessing Record<string, Students[]>. Which IMHO should be the default behavior for Record<>

This MR removes the Partial<> wrap in the return to account for this undesired behavior.

@Harris-Miller Harris-Miller changed the title fix(groupBy) do not return wrapped in Partial<> Draft: fix(groupBy) do not return wrapped in Partial<> Feb 6, 2024
@Harris-Miller
Copy link
Collaborator Author

Closing per #91 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant