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

Bug: [member-ordering] natural-sort should be implemented using String's native capabilities instead of a 3rd party package #5989

Closed
julienw opened this issue Nov 14, 2022 · 8 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin refactor PRs that refactor code only wontfix This will not be worked on

Comments

@julienw
Copy link

julienw commented Nov 14, 2022

I left a message on the PR that implemented this rule => #5662 (comment)
But I thought it was better to file a new issue instead.

I saw that #5662 implemented a natural-sort-ordering option, which sounds good. However this can be implemented using node's native API instead of relying on a 3rd-party lib.

Would you be open to a PR for this?

@julienw julienw added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Nov 14, 2022
@JoshuaKGoldberg
Copy link
Member

Interesting! Which native API are you referring to?

@JoshuaKGoldberg JoshuaKGoldberg added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Nov 14, 2022
@julienw
Copy link
Author

julienw commented Nov 15, 2022

ooops, sorry, I should have mentioned it here. I mentioned it in the message that I linked :)

I'm refering to the native JS API String.prototype.localeCompare that node supports for a long time. I tried a node version as old as v10 locally. Here is a very simple code example:

["a2", "a10", "a1"].sort((a, b) => a.localeCompare(b, undefined, { numeric: true }))
// Result: [ 'a1', 'a2', 'a10' ]

I'm thinking that probably instead of undefined we should use 'en-US' so that it's locale-independent.

Hope this helps

@JoshuaKGoldberg JoshuaKGoldberg changed the title Bug: [member-ordering] natural-sort should be implemented using node's native capabilities instead of a 3rd party package Bug: [member-ordering] natural-sort should be implemented using String's native capabilities instead of a 3rd party package Nov 15, 2022
@JoshuaKGoldberg
Copy link
Member

Ah thanks - so this actually isn't Node-specific, it's JS strings in general. That threw me - we support running in browsers and therefore try not to rely on Node-specific APIs.

I tried the suggested change out locally and saw a couple of incorrect errors. For example:

Member a5 should be declared before member a10.

The natural-compare-lite package compares groups of numbers against each other numerically, not alphabetically. That's not the same as using localCompare.

Am I missing something?

@julienw
Copy link
Author

julienw commented Nov 15, 2022

I'd like to see your code :) Especially have you used {numeric: true} as an option? That's the secret sauce.

@JoshuaKGoldberg
Copy link
Member

// import naturalCompare from 'natural-compare-lite';

function naturalCompare(a: string, b: string): number {
  return a.localeCompare(b, 'en-US', { numeric: true });
}

We've been strapped for time recently so I'm going to close this. But if you can make a draft PR that shows existing test cases passing with localeCompare, we can reopen. 🙂

@JoshuaKGoldberg JoshuaKGoldberg added wontfix This will not be worked on refactor PRs that refactor code only and removed bug Something isn't working awaiting response Issues waiting for a reply from the OP or another party labels Nov 15, 2022
@julienw
Copy link
Author

julienw commented Nov 15, 2022

My understandng:
natural-compare-lite orders with upper case letters first
localeCompare orders with lower case letters first

Normally we can configure this in localeCompare using caseFirst: "upper" but... I couldn't make it work (not in node, not in Firefox).

We can also decide that localeCompare order is the right one -- this is just a choice after all.

I asked around to colleagues about the issue though.

@JoshuaKGoldberg
Copy link
Member

Sounds good! We can always look at an issue for new sort options if you feel there's a variant that should be added.

@julienw
Copy link
Author

julienw commented Nov 16, 2022

I got the bottom of this.

  1. localeCompare sorts lexicographically. This means that A and a always come before B and b.
  2. the caseFirst option configures whether A comes before or after a. The default is locale-dependent.

Therefore with localeCompare we can't have both the numeric sorting and have all uppercases come before lowercases like the 3rd-party library does.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin refactor PRs that refactor code only wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants