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

fix(collection)!: default sort comparison algorithm #10412

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Jul 28, 2024

Please describe the changes this PR makes and why it should be merged:

The documentation for Collection#sort and Collection#toSorted implies that if no comparison function is passed to the method, the default comparison behaviour mirrors that of Array.prototype.sort – in other words, stringwise comparison by UTF-16 code points.

As things stand, that's not the case. There are a couple of significant issues with the current implementation of defaultSort:

  • The function doesn't implement stringwise comparison but instead just uses the > operator, which preferentially performs numeric comparison of its operands.
    • For example, '1.0' is sorted after 1 with stringwise comparison, but '1.0' > 1 is false.
  • The function returns -1 (ie. A before B) if A is neither greater than B nor strict-equal to B. However, this isn't logically correct in a sort comparison context, and leads to violations of the required properties of a reliable comparator.
    • For example:
      const a = { a: 1 };
      const b = { b: 2 };
      defaultSort(a, b); // -1
      defaultSort(b, a); // also -1

I'm assuming that the intention is for the default behaviour to mirror that of Array.prototype.sort, and so this PR rewrites the function to follow the ECMA-262 specification for the default comparison algorithm. The docblocks are also updated to reflect this more accurately.

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Copy link

vercel bot commented Jul 28, 2024

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

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Jul 28, 2024 0:12am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Jul 28, 2024 0:12am

Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.76%. Comparing base (bf6761a) to head (d6945c8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10412      +/-   ##
==========================================
+ Coverage   50.73%   50.76%   +0.03%     
==========================================
  Files         228      228              
  Lines       20646    20659      +13     
  Branches     1253     1256       +3     
==========================================
+ Hits        10475    10488      +13     
  Misses      10127    10127              
  Partials       44       44              
Flag Coverage Δ
collection 100.00% <100.00%> (ø)
proxy 78.52% <ø> (ø)
rest 92.68% <ø> (ø)
ws 51.88% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/collection/__tests__/collection.test.ts Outdated Show resolved Hide resolved
@kyranet
Copy link
Member

kyranet commented Jul 28, 2024

I'm labelling this as semver-major because it changes the behaviour of Collection#sort without arguments, which may have been desirable to users (and not longer is anymore).

@Renegade334 Renegade334 requested a review from kyranet July 28, 2024 12:17
@ckohen ckohen changed the title fix(collection): default sort comparison algorithm fix(collection)!: default sort comparison algorithm Jul 28, 2024
@almeidx almeidx added this to the collection 3.0.0 milestone Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review Approved
Development

Successfully merging this pull request may close these issues.

5 participants