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

feat(keyBy): Add keyBy #93

Merged
merged 3 commits into from
Jun 30, 2024
Merged

feat(keyBy): Add keyBy #93

merged 3 commits into from
Jun 30, 2024

Conversation

mass2527
Copy link
Contributor

This PR resolves #84.

Question and Consideration

For consistency, I referenced the groupBy function since it bears similarity to keyBy. This is why I used almost the same signature for keyBy as for groupBy, except for the return type:

function groupBy<T, K extends string>(arr: readonly T[], getKeyFromItem: (item: T) => K): Record<K, T[]>;
function keyBy<T, K extends string>(arr: readonly T[], getKeyFromItem: (item: T) => K): Record<K, T>;

While referencing, I became curious about why type K is restricted to string and not PropertyKey. This restriction requires developers to coerce keys of types such as number into string.

const array = [
      { score: 1, name: 'John' },
      { score: 2, name: 'Jane' },
      { score: 1, name: 'Joe' },
    ];

const result = groupBy(array, item => item.score.toString());
const result2 = keyBy(array, item => item.score.toString());

I believe it would enhance usability if developers using the keyBy function did not have to handle type conversions differently based on the key type.

Is there a specific reason that I haven't considered?

Benchmark

스크린샷 2024-06-29 오후 11 36 29

@mass2527 mass2527 requested a review from raon0211 as a code owner June 29, 2024 15:43
Copy link

vercel bot commented Jun 29, 2024

@mass2527 is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@codecov-commenter
Copy link

codecov-commenter commented Jun 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (5377450) to head (b732aec).
Report is 361 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #93   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           55        56    +1     
  Lines          290       296    +6     
  Branches        34        34           
=========================================
+ Hits           290       296    +6     

Copy link

codspeed-hq bot commented Jun 29, 2024

CodSpeed Performance Report

Merging #93 will degrade performances by 85.09%

Comparing mass2527:feat/keyBy (b732aec) with mass2527:feat/keyBy (dada0d4)

Summary

⚡ 4 improvements
❌ 9 regressions
✅ 83 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark mass2527:feat/keyBy mass2527:feat/keyBy Change
lodash 49.2 µs 330.1 µs -85.09%
lodash/differenceBy 70.6 µs 224.9 µs -68.61%
lodash/differenceWith 341.5 µs 224.9 µs +51.87%
lodash/intersectionBy 308.6 µs 466.5 µs -33.85%
lodash/isNil 35.6 µs 190 µs -81.27%
lodash/pick 86.5 µs 254.8 µs -66.05%
es-toolkit/range 48.3 µs 39.4 µs +22.64%
lodash/range 44.1 µs 33.4 µs +31.92%
lodash/union 75.6 µs 353.9 µs -78.64%
lodash/unionBy 75.4 µs 229.2 µs -67.08%
lodash/unionWith 84.8 µs 245.7 µs -65.48%
es-toolkit/uniqBy 17.3 ms 6.9 ms ×2.5
lodash/xorBy 91.4 µs 406.1 µs -77.51%

@raon0211
Copy link
Collaborator

Thanks for your contribution! Since this is working as expected, I will merge this pull request.

Meanwhile, I agree that all keys might not be numbers. I guess we should accept numbers and symbols as keys. Could you open a seperate pull request for that?

@raon0211 raon0211 merged commit 184ce17 into toss:main Jun 30, 2024
7 of 8 checks passed
@mass2527 mass2527 deleted the feat/keyBy branch June 30, 2024 04:52
@mass2527
Copy link
Contributor Author

Thanks for your contribution! Since this is working as expected, I will merge this pull request.

Meanwhile, I agree that all keys might not be numbers. I guess we should accept numbers and symbols as keys. Could you open a seperate pull request for that?

Yes, I am going to make a PR for this.

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.

Support for keyBy
4 participants