Skip to content

fix[orderBy, sortBy]: keep sort order consistent when values contain NaN - #2133

Open
minseonkkim wants to merge 2 commits into
toss:mainfrom
minseonkkim:fix/sortby-orderby-nan
Open

minseonkkim wants to merge 2 commits into
toss:mainfrom
minseonkkim:fix/sortby-orderby-nan

Conversation

@minseonkkim

Copy link
Copy Markdown
Contributor

Summary

When the sort key contains NaN, orderBy and sortBy leave the array partially unsorted.

import { sortBy } from 'es-toolkit';

const items = [{ v: 3 }, { v: NaN }, { v: 1 }, { v: 2 }, { v: 0 }];

sortBy(items, ['v']).map(item => item.v);
// Expected: [0, 1, 2, 3, NaN]
// Actual:   [3, NaN, 0, 1, 2]

compareAscending in src/_internal/compareValues.ts compares regular values with < and >. Every comparison involving NaN returns false, so NaN is treated as equal to every value. This breaks the transitivity the comparator relies on (3 == NaN, NaN == 0, but 3 > 0), and the sort produces an inconsistent result. Elements before the NaN are not sorted at all.

This can easily happen in practice, for example when a numeric field comes from a failed parseFloat or a 0 / 0 calculation.

I found this while reading the code, so there is no related issue.

Changes

  • src/_internal/compareValues.ts: give NaN its own rank, so it never reaches the < / > comparison. The order is now regular values < NaN < null < undefined, following the existing approach for null and undefined.
  • src/array/orderBy.spec.ts: add tests for ascending, descending, and mixed NaN / null / undefined cases.

The new tests fail on main and pass with this change.

I placed NaN before null and undefined because NaN is still a number, while null and undefined mean the value is missing. Lodash uses a different order (null < undefined < NaN), so I'm happy to change it if you prefer that.

es-toolkit/compat has its own compareValues, so it is not affected.

Benchmark results

main

 BENCH  Summary

   benchmarks  es-toolkit/orderBy - orderBy.bench.ts > orderBy
    1.77x faster than es-toolkit/compat/orderBy
    1.85x faster than lodash/orderBy

   benchmarks  es-toolkit/compat/orderBy - orderBy.bench.ts > orderBy (nested property names)
    1.41x faster than lodash/orderBy

   benchmarks  es-toolkit/compat/orderBy - orderBy.bench.ts > orderBy (property path)
    1.23x faster than lodash/orderBy

   benchmarks  es-toolkit/sortBy - sortBy.bench.ts > sortBy
    1.62x faster than es-toolkit/compat/sortBy
    1.74x faster than lodash/sortBy

   benchmarks  es-toolkit/compat/sortBy - sortBy.bench.ts > sortBy (nested property names)
    1.47x faster than lodash/sortBy

   benchmarks  es-toolkit/compat/sortBy - sortBy.bench.ts > sortBy (property path)
    1.13x faster than lodash/sortBy

This branch

 BENCH  Summary

   benchmarks  es-toolkit/orderBy - orderBy.bench.ts > orderBy
    2.07x faster than es-toolkit/compat/orderBy
    2.10x faster than lodash/orderBy

   benchmarks  es-toolkit/compat/orderBy - orderBy.bench.ts > orderBy (nested property names)
    1.39x faster than lodash/orderBy

   benchmarks  es-toolkit/compat/orderBy - orderBy.bench.ts > orderBy (property path)
    1.33x faster than lodash/orderBy

   benchmarks  es-toolkit/sortBy - sortBy.bench.ts > sortBy
    1.67x faster than es-toolkit/compat/sortBy
    1.84x faster than lodash/sortBy

   benchmarks  lodash/sortBy - sortBy.bench.ts > sortBy (nested property names)
    1.14x faster than es-toolkit/compat/sortBy

   benchmarks  es-toolkit/compat/sortBy - sortBy.bench.ts > sortBy (property path)
    1.19x faster than lodash/sortBy

@vercel

vercel Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
es-toolkit Error Error Oct 4, 2026 6:49am UTC

Request Review

This branch had an error being deployed

1 failed deployment
Preview — ba9190b2 Deployed Oct 4, 2026 by vercel[bot]
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.

1 participant