Repository navigation
Array.prototype.sort make compareFn required in lib.d.ts #18286
Description
This is a suggestion to change the signature declaration of Array sort in lib.d.ts from
sort(compareFn?: (a: T, b: T) => number): this;to
sort(this: string[], compareFn?: (a: string, b: string) => number): this;
sort(compareFn: (a: T, b: T) => number): this;i.e. make compareFn non-optional for non-string arrays. This would result in errors in cases such as
[2, 10].sort() // returns [10, 2], i.e. sorted lexicographically, which is very unintuitive.While this is strictly speaking a breaking change, it wouldn't be the first time the typings are stricter than the actually definitions.
Activity
zspitz commentedon Sep 7, 2017
Is this part of Typescript's mandate, to make built-in Javascript APIs safer to use?
DanielRosenwasser commentedon Sep 7, 2017
We've done it in certain locations, like
parseInt.aluanhaddad commentedon Sep 7, 2017
Seems reasonable. I do the same special casing for
string[]when I write adistinctfunction.kitsonk commentedon Sep 7, 2017
As far as the goal part, it feels like goal:
Personally, I can't conceive of a situation where this would be the desired functionality. TypeScript already prevents illogical comparisons and several other challenges. While this is slightly higher order, it does feel very much in the spirit of TypeScript. The only downside is that the error message can't provide any additional information as to why it is an error. Of course the JSDoc could provide some information in the overload as to why it is a bad idea.
NaridaL commentedon Sep 20, 2017
Support seems to be unanimous, if you tag this as Accepting PRs I'll submit one.
RyanCavanaugh commentedon Sep 20, 2017
Daniel Rosenwasser (@DanielRosenwasser) is going to try this and see what breaks
RyanCavanaugh commentedon Aug 15, 2018
Daniel Rosenwasser (@DanielRosenwasser) dooooo iiiittttt break the world go go go
7 remaining items
iansan5653 commentedon Dec 6, 2019
I'd like to see this as well - here's an example where this mistake made it into production: microsoft/vscode#86440
cincodenada commentedon Jan 4, 2022
I came here because I'm working in a non-TypeScript codebase at work that I would love to see migrate towards TypeScript. Recently this exact bug - calling
sort()on a numeric array and assuming it sorted numerically - made it into production and caused some minor havoc, and it would have been nice to use it as an example of what reasonable typing could do for us! So, count this as a vote from a passer-by in favor as well.I think this is a very common bug, and this change does seem very much in line with TypeScript's first goal, albeit perhaps at the expense of goals 7 and 11. I would love to see this make it in - I expect that the number of bugs prevented would vastly outnumber the earnest use cases broken, and for those use cases there is a simple workaround, something like
(a, b) => String(a).localeCompare(b)as acompareFn.This is in my opinion better practice anyway - it makes it unambiguous that yes, you really do want to sort the numbers lexicographically. I see above mentioned there's no opportunity to provide a helpful error message, but the, JSDoc would be better than nothing, and could perhaps suggest
localeCompare()for the cases where you do need it.