Feature: drop javascript-natural-sort dep

[fbartho]

This PR removes our reliance on natural-sort, an npm package last updated 9 years ago!

Instead this uses String.localeCompare(…) which is a built-in method supported on all Node versions we target. Further this is a standard String api, so some shim implementation will be present even if the user isn't running on Node! (eg. deno or other runtimes)

Original PR Description Below

Minor problem: localeCompare appears to always interleave capital & lowercase letters.

I don't know how to do both numeric-sort AND case-sensitive (classic Array.sort() mode) at the same time using pure locale-compare.
This is how I was forced to replicate the original case-sensitive sort:

prettier-plugin-sort-imports/src/natural-sort/index.ts

Lines 14 to 16 in 5f07e19

	if (!importOrderCaseInsensitive) {
	return left < b ? -1 : left > b ? 1 : 0;
	}

Relates to: #23 (comment)

[IanVS]

In general I'm in favor of relying on native JS apis when possible, but I have some concern that the behavior here could be different from javascript-natural-sort, and we could introduce some unintentional breaking changes.

I think we can use the relevant test cases from https://github.com/Bill4Time/javascript-natural-sort/blob/master/unit-tests.html to minimize that chance, but since I don't think this fixes #23, and is really just a dependency cleanup, let's hold off on merging it until we're ready to release v4, which I think will be relatively soon. What do you think?

[IanVS]

Thinking out loud. Are we certain we always want to sort by english locale? I think so, because otherwise users working together on a shared project in different locales could cause formatting churn.

[fbartho]

At the time when I wrote this, I thought that we could hardcode ‘en’ because I was only considering detecting locale relevant here for currency, dates, and numeric separators. Of those, I only cared about the numeric separators: 1,000.00 vs 1.000,00 — and I arbitrarily assumed that those (separators) were irrelevant for js module names, and irrelevant for file names. (Implying we only really care about sorting short integers at the beginning or end of file names)

Further research shows that the locale actually affects how accented characters sort related to lower/uppercase letters.

I think it’d be fair to argue one of the following

• that our opinionated library could assume that those are unsupported, because file names / to limit complexity
• that our opinionated library only looks at your node’s default locale (risks “doesn’t work the same on my machine”)
• must have an additional locale parameter in config. :(

[fbartho]

In general I'm in favor of relying on native JS apis when possible, but I have some concern that the behavior here could be different from javascript-natural-sort, and we could introduce some unintentional breaking changes.

@IanVS Yes, I’m suggesting we target this for the breaking release. In that case, we don’t even need to add a large test suite. If we can decide how to resolve this topic: #28 (comment) then we can add a few targeted tests, and say deeper testing belongs to Intl.Collator.

(Principle: Don’t write exhaustive tests verifying your OS or framework is doing what its contract offers — unless its mission critical).

[fbartho]

@IanVS — note: either #23 was already fine, because modern NodeJS uses modern V8 which has Stable Sort now, or JavaScript-natural-sort wasn’t stable, in which case this ticket does fix #23.

Within any chunk, tie breaking should have a deterministic outcome once when you use a stable sorting algorithm with a defined comparator (edge cases exist, but don’t apply here).

So… either this ticket fixes #23, or #23 is moot, but either way, I support shipping this with no-options in the breaking release. (Caveat locale decision).

[IanVS]

I don't think that stable sorting fixes issue #23. For example, if you add this test case to this branch's natural-sort.spec.ts, it will fail:

it('should sort deterministically if case-insensitive', () => {
    expect(
        ['./something', './Something'].sort((a, b) =>
            naturalSort(a, b, { importOrderCaseInsensitive: true }),
        ),
    ).toEqual(['./something', './Something']);

    expect(
        ['./Something', './something'].sort((a, b) =>
            naturalSort(a, b, { importOrderCaseInsensitive: true }),
        ),
    ).toEqual(['./something', './Something']);
});

The order in which the imports were originally defined determines what order they are placed in (which is what it means to have a stable sort). This is not what we want, however. We want a deterministic sort. Given two imports, in any order, they should always have the same final order.

Maybe we are not agreeing on what it means to be case insensitive. To me, it means, don't worry about whether capitals come before lowercase, or vise versa. So, Z-dep shouldn't come before a-dep. But, given A-dep and a-dep, we need an answer for which should come first, since in a case-insensitive sort, they're exactly the same.

[fbartho]

Maybe we are not agreeing on what it means to be case insensitive. To me, it means, don't worry about whether capitals come before lowercase, or vise versa. So, Z-dep shouldn't come before a-dep. But, given A-dep and a-dep, we need an answer for which should come first, since in a case-insensitive sort, they're exactly the same.

Ah! That indeed is our difference. localeCompare in insensitive mode says (a-dep, A-dep) = 0, (A-dep, a-dep) = 0 so either order is identical. To me that's internally consistent with "insensitive", so that's what I've been visualizing.

[IanVS]

Given that we've cleared up the confusion over #23, I think we can close out this PR. But, let me know if you disagree and we can discuss further. Thanks for your work on this, and sorry for the miscommunication.

[IanVS]

Given that you and @blutorange believe the best option is to avoid dictating the order of imports differing only by case sensitivity, I think it's worth re-visiting this.

[fbartho]

@IanVS I pushed an update to this that made it compatible with next as of 9am, but lol, I see there's already more conflicts. I'll wait to hear if you want more from me on this or if you want to take it over!

[IanVS]

Whoops, sorry I hadn't seen that you pushed that. Let's finish the option cleanup and then come back to this afterwards, if that's cool with you?

[fbartho]

However you prefer works for me. Was just trying to be helpful :P

For what it's worth, I think it's just a trivial conflict if you want to fold this PR in and forget about it!

[IanVS]

Alright, in that case I'll check it out now, thanks.

[IanVS]

Thanks for bearing with me on this one. Glad we can finally get this cleanup in.