azat-io / eslint-plugin-perfectionist

☂️ ESLint plugin for sorting various data such as objects, imports, types, enums, JSX props, etc.
https://perfectionist.dev
MIT License
2.14k stars 44 forks source link

Bug: Upgrading to v4, named imports are getting rearranged #384

Closed aeharding closed 1 week ago

aeharding commented 1 week ago

Describe the bug


With the following v3.9.1:

"perfectionist/sort-named-imports": [
  "warn",
  { ignoreCase: false, type: "natural", ignoreAlias: false },
],
import { LOCALSTORAGE_KEYS, get } from "#/features/settings/syncStorage";

✅ Valid


v4.0.2:

Expected "get" to come before "LOCALSTORAGE_KEYS".eslint[perfectionist/sort-named-imports](https://perfectionist.dev/rules/sort-named-imports)


Code example

See above

ESLint version

v9.15.0

ESLint Plugin Perfectionist version

v4.0.2

Additional comments

No response

Validations

hugop95 commented 1 week ago

Hi @aeharding,

I haven't taken a deeper look yet, but I believe that this is likely due to the following breaking change:

The implementation of natural sort is different, particularly when talking about lowercase/uppercase characters. I think that there is no way to make uppercased characters go before lowercased ones with this implementation.

hugop95 commented 1 week ago

Confirmed:

natural-orderby:

console.log(naturalCompare('LOCALSTORAGE_KEYS', 'get')) // 1

natural-lite:

console.log(naturalCompare('LOCALSTORAGE_KEYS', 'get')) // -1
console.log(naturalCompare('lOCALSTORAGE_KEYS', 'get')) // 1
azat-io commented 1 week ago

This seems to be a question for natural-orderby. Perhaps we should create an issue about case sensitivity support for this project.

Sparticuz commented 1 week ago

Another difference that came up for me was that it seemed to put special characters before, so eslint-comments.js was moved before eslint.js

Before:

import eslintJSConfig from "./configs/eslint.js";
import eslintCommentsConfig from "./configs/eslint-comments.js";

After:

import eslintCommentsConfig from "./configs/eslint-comments.js";
import eslintJSConfig from "./configs/eslint.js";
sthenault commented 5 days ago

@Sparticuz did you find a way to get back to original behavior? Just updated to 4.1.2 and hit the same problem. Tried to update config to stop using "natural" sorting :

        'perfectionist/sort-imports': [
            'error',
            {
                type: 'alphabetical',
                order: 'asc',
                groups: ["builtin", "external", "parent", "sibling", "index"],
                ignoreCase: false,
            },
        ],

but then I still have errors such as:

  9:1  error  Expected "../components/data2ResultWrapper" to come before "../components/LogsTable"  perfectionist/sort-imports
sthenault commented 5 days ago

ooops wrong ping BTW, rather meant to call for @aeharding feedback

sthenault commented 5 days ago

may be related somewhat to

l = ["a", "A"]
l.sort() -> [ "A", "a" ]
l.sort((a,b) => a.localeCompare(b)) ->  [ "a", "A" ]

? As it doesn't seem to have option providing backward compat, I suppose I have to update my imports order?

Wondering how this may have been unreported yet so I'm may be mistaken somewhere, though it seems that "sort-import" tests don't cover ignoreCase: false.

hugop95 commented 5 days ago

@sthenault There is a discussion regarding this in #375.

There is no regression with the alphabetical sort and ignoreCase: it's just that the way ignoreCase and localeCompare work together is misleading: it has almost no effect with this sorting algorithm.

Ultimately, the issue is that all of those different sorting algorithms are opinionated, and sometimes, options are not enough to customize them: some users would like to have capital letters take priority over lowercased ones, others would like certain special characters before others, so I'm aiming to offer a way to add custom sorting algorithms.

sthenault commented 5 days ago

@hugop95 thank you for your reply. I understand the problem under the cover now. If I use 'alphabetical' instead of 'natural' with 3.9.1, then it also considers that 'a' should come before 'A', while 'natural' expect the opposite... There is indeed no regression in v4 on this topic.

Maybe because I'm too old and expect ascii order I missed that possibility ;)