fix(collection)!: default sort comparison algorithm
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 after1with stringwise comparison, but'1.0' > 1is false.
- For example,
- 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
- For example:
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
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 |
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.
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).
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!