discord.js icon indicating copy to clipboard operation
discord.js copied to clipboard

fix(collection)!: default sort comparison algorithm

Open Renegade334 opened this issue 1 year ago • 3 comments

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

Renegade334 avatar Jul 28 '24 00:07 Renegade334

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 Nov 30, 2024 3:01pm
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Nov 30, 2024 3:01pm

vercel[bot] avatar Jul 28 '24 00:07 vercel[bot]

Codecov Report

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

Project coverage is 36.99%. Comparing base (a696005) to head (941022f). Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10412      +/-   ##
==========================================
+ Coverage   36.96%   36.99%   +0.02%     
==========================================
  Files         239      239              
  Lines       15284    15290       +6     
  Branches     1378     1381       +3     
==========================================
+ Hits         5650     5656       +6     
  Misses       9619     9619              
  Partials       15       15              
Flag Coverage Δ
collection 100.00% <100.00%> (ø)
proxy 64.63% <ø> (ø)
rest 85.85% <ø> (ø)
ws 34.45% <ø> (ø)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jul 28 '24 00:07 codecov[bot]

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).

kyranet avatar Jul 28 '24 07:07 kyranet

Please resolve conflicts!

Also, would you mind writing a BREAKING CHANGE entry in a commit or in the pull request's body somewhere (https://github.com/discordjs/discord.js/blob/main/.github/COMMIT_CONVENTION.md)? Thank you!

Jiralite avatar Nov 29 '24 09:11 Jiralite