toss / es-toolkit

A modern JavaScript utility library that's 2-3 times faster and up to 97% smaller—a major upgrade to lodash.
https://es-toolkit.slash.page
Other
6.25k stars 263 forks source link

fix(compat/orderby): add missed test cases and fix the problematic parts #427

Closed dayongkr closed 2 weeks ago

dayongkr commented 3 weeks ago

Description

compat/orderby missed including these testcases(https://github.com/lodash/lodash/blob/6a2cc1dfcf7634fea70d1bc5bd22db453df67b42/test/sortBy-methods.spec.js) and hidden case(only implemented). And it also missed some of feature, so I added.

! Important

Currently, we had a bug that makes by below code:

// src/compat/array/orderBy.ts:81
keys = keys.map(key => getPath(key, collection[0]));

Before the changes, it checked the key is not a deep path only with the first element of the collection. So if the first element is different with others, then throw the TypeError like this:

orderBy([{ a: 1 }, { 'a.b': 3 }, { 'a.b': 1 }], ['a.b'], ['asc']) // TypeError: Cannot read properties of undefined (reading 'b')
orderBy([{ 'a.b': 3 }, { a: 1 }, { 'a.b': 1 }], ['a.b'], ['asc']) // success

So I changed the logic of checking keys similar with lodash(slower but safer).

Benchmarks

Screenshot 2024-08-25 at 12 16 31 AM

The performance result is changed, you can check result by a comment.

vercel[bot] commented 3 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 29, 2024 2:05am
codecov-commenter commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.77%. Comparing base (b9966aa) to head (8258b85).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/toss/es-toolkit/pull/427/graphs/tree.svg?width=650&height=150&src=pr&token=8N5S3AR3C7&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss)](https://app.codecov.io/gh/toss/es-toolkit/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) ```diff @@ Coverage Diff @@ ## main #427 +/- ## ======================================= Coverage 99.77% 99.77% ======================================= Files 165 165 Lines 1325 1334 +9 Branches 357 360 +3 ======================================= + Hits 1322 1331 +9 Misses 2 2 Partials 1 1 ```
dayongkr commented 3 weeks ago

I Improved performance by preparing all cases of criterion before comparing.

At now, we don't need to use isKey (heavy function) each criterion.

N: length of collection, K: length of criteria O(NK) -> 0

And also we convert deep path to nested path at once. Before code converted each collection items.

N: length of collection, K: length of criteria O(NK) -> O(K)

Before

Screenshot 2024-08-25 at 11 49 30 AM

After

Screenshot 2024-08-25 at 11 51 07 AM
dayongkr commented 2 weeks ago

Thanks for your great work!

After merging this PR, I am going to implement compat/sortBy based on compat/orderBy!