[import/order] Add ability to sort groups in order of nested groups

[Pearce-Ropion]

Resolves #2685, #2683, #2682

This PR changes 2 things:

1. Add the ability to sort groups in the order of nested groups
2. Add the ability to sort types in the order of the rest of the groups array.

Both of these features are currently disabled by default and must be enabled with the intraGroupOrdering option. I've added a TODO: semver major flag to this option indicating to make this behavior the default in the next major version. We can either remove the flag after that or enable its behavior by default with the option to disable it.

Add the ability to sort groups in the order of nested groups (intraGroupOrdering = true)
Will mutate ranks of a single group based on the order of the of the group's nested groups.

If the rule is configured to alphabetize, it will alphabetize each subgroup individually and then sort the subgroups in nested groups order. That is to say, if the rule is configured as [['parent', 'sibling']], it will sort all of the parent imports alphabetically, then sort all of the sibling imports alphabetically. Then move all the alphabetized parent imports before the alphabetized sibling imports.

Ex.
Given the following code and rule options

// code
import fs from 'fs';
import { foo } from './foo';
import { bar } from './bar';
import { fooz } from '../fooz';

// options
{
  intraGroupOrdering: true,
  groups: ['builtin', ['parent', 'sibling']],
  newlines: 'always',
  alphabetize: {
    order: 'asc',
  }
}

Results in: ../fooz being before ./foo because parent is listed before sibling in the groups array

import fs from 'fs';

import { fooz } from '../fooz';
import { bar } from './bar';
import { foo } from './foo';

Add the ability to sort types in the order of the rest of the groups array (intraGroupOrdering = true)
Will mutate ranks of all type imports such that they are sorted in the flattened order of the original groups array without the types. If the rule is configured to alphabetize, it will do the same thing as how alphabetization works above.

Ex.
Given the following code and rule options

// code
import type { Dirent } from 'fs';
import type { Foo } from './foo';
import type { Bar } from './bar';
import type { Fooz } from '../fooz';

import fs from 'fs';
import { fooz } from '../fooz';
import { bar } from './bar';
import { foo } from './foo';

// options
{
  intraGroupOrdering: true,
  groups: ['type, 'builtin', ['parent', 'sibling']],
  newlines: 'always',
  alphabetize: {
    order: 'asc',
  }
}

Results in: ../fooz being before ./foo because parent is listed before sibling in the groups array

import type { Dirent } from 'fs';
import type { Fooz } from '../fooz';
import type { Foo } from './foo';
import type { Bar } from './bar';

import fs from 'fs';

import { fooz } from '../fooz';
import { bar } from './bar';
import { foo } from './foo';

Testing

I had a lot of trouble with tests. @ljharb I'm not sure if you've seen this before but I was randomly getting cases where the groups array was setting elements to undefined. I assumed the code was mutating the groups array somewhere but I couldn't find it. Please be on the lookout for that. Also, I would randomly need newlines in the output of some tests but not for others.

I decided I would rather get the PR up so that I can show progress than continue struggling with the system. I think there should be more testing for the case where both intraGroupOrdering and alphabetize is enabled. As well as tests for unknown types when this option is enabled. The tests I did add are very basic. I am open to test suggestions.

[notaphplover]

Hi @Pearce-Ropion, thank you so much for the PR. Just a little suggestion: would you mind not using the Resolves keyword? In my humble opinnion the merge of this PR does not guarantee that a lot of corner cases not covered here will be solved. Wouldn't it be good to wait for the user feedback before simply closing the issues?

Regarding tests, man I don't know what to say. In theory https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md is the source of truth here, so you should add enough cases to guarantee the code complains that doc, but the test file is massive so it's really hard to tell which cases are already covered and which ones not. This is a reason to not auto close the issues.

Once this is finished I would really consider a refactor, there seem to be a lot of complexity in both the source and the test files. Having said that, I'm a mere spectator, it's up to you guys to form your own opinnion and reach your own conclussions.

[ljharb]

The way to know it fixes the linked issues is to add those test cases here - which shouldn't interact with new options at all. @Pearce-Ropion, could we do that? (note i haven't looked at the diff yet, so maybe you already have!)

[Pearce-Ropion]

Hi @Pearce-Ropion, thank you so much for the PR. Just a little suggestion: would you mind not using the Resolves keyword? In my humble opinnion the merge of this PR does not guarantee that a lot of corner cases not covered here will be solved. Wouldn't it be good to wait for the user feedback before simply closing the issues?

I think that is the point of being able to discuss the change in the PR and add/remove test cases. Ideally a PR should resolve issues. Issues can always be re-opened if a PR does not adequately address the problems. However, in my opinion, this change does resolve all of the listed issues.

Now, I agree that there are potentially a lot of corner cases that I am not covering, but that is why I explicitly state that I think I need to add more test cases because I know I am not covering all of the edge cases. Is there a particular issue with the proposed changes that you think does not resolve the issues listed? If so, please bring it up here so that we can discuss it and potentially make changes as needed.

Also, I am not entirely sure how this repo is set up, but I am pretty sure @ljharb manually closes issues as they are resolved.

[notaphplover]

I think that is the point of being able to discuss the change in the PR and add/remove test cases.

I totally agree with you, I couldn't be happier after reading this :)

Now, I agree that there are potentially a lot of corner cases that I am not covering, but that is why I explicitly state that I think I need to add more test cases because I know I am not covering all of the edge cases. Is there a particular issue with the proposed changes that you think does not resolve the issues listed? If so, please bring it up here so that we can discuss it and potentially make changes as needed.

To be honest, understanding your changes and giving feedback on top of those seems quite challenging to me given my experience with this code base and the complexity surrounding these files. I think the library maintainers are way more capable than me. Having said that, I can take a different approach: I can give you test cases so we can see what's going on with the logic (asuming the cases are right, of course).

In order to start, what do you think about the following case? It complains the document rules but throws an error:

test({
      code: `import { beforeAll, describe, expect, it, jest } from '@jest/globals';

import { ServiceId, Tag } from '@cuaklabs/iocuak-common';
import { Binding, BindingType } from '@cuaklabs/iocuak-models';

import { TypeBindingFixtures } from '../fixtures/TypeBindingFixtures';
import { BindingService } from './BindingService';
import { BindingServiceImplementation } from './BindingServiceImplementation';
`,
      options: [{
        alphabetize: {
          order: 'asc',
          caseInsensitive: true,
        },
        groups: ['builtin', 'external'],
        'newlines-between': 'always',
        pathGroups: [
          {
            pattern: '@jest/globals',
            group: 'external',
            position: 'before',
          },
          {
            pattern: 'jest-mock',
            group: 'external',
            position: 'before',
          },
        ],
        pathGroupsExcludedImportTypes: ['@jest/globals', 'jest-mock'],
      }],
    }),

According to the output, the error is:

`../fixtures/TypeBindingFixtures` import should occur after import of `./BindingServiceImplementation`

But this error is a regression. Until eslint-plugin-import version 2.26.0, a grandparent import was before a parent import. Is there a reason to change this behavior? Is that the expected behavior? If so, should we update docs and changelog according to this behavior? This way any user would know it's not a bug but the new expected behavior.

Also, I am not entirely sure how this repo is set up, but I am pretty sure @ljharb manually closes issues as they are resolved.

If that's the repo's methodoly let's proceed that way.

[Pearce-Ropion]

Ok, I think I am started to understand your issue with using "resolves" in this PR. I think I definitely could have explained how this PR resolves the given issues better. I think the main problem with "resolving" these issues is that some of them are technically unresolvable without further intervention by @ljharb.

This PR adds an option (intraGroupOrdering) that enables user the ability to resolve these issues on their own. So in that regard, it could be perceived as a feature/enhancement rather than a bug fix. The central problem that all of the given issues revolve around is that the way that alphabetization of imports in this package has changed between v2.26 and v2.27. I think what you and those that reported these issues are calling a "bug" should probably have been listed as a breaking change in the changelog.

Essentially, #2396 changes the way that alphabetization is handled by making it so that each path segment of a given path (split on /) is sorted in alphabetical order. So this means that . is sorted before .. given the following paths: ../foo and ./foo. Before #2396 the whole path was used for alphabetization. So ../foo was sorted before ./foo.

Ultimately, there isn't a straightforward fix to make it so that ../foo is sorted before ./foo because when alphabetically sorted by path segment, the current order is correct. Also, unless your specifically tell the rule to sort parent imports before sibling imports, the rule is under no obligation to do so. Any sorting of that nature that previously occurred (in v2.26 and below) was just a happy accident. So therefore, you could potentially make the argument that the way that alphabetical sorting worked in the previous implementation (ie v2.26 and below) was actually a bug and that the way it works in newer versions (v2.27 and above) is correct.

The changes made in #2396 were not made by me and therefore is not up to me to decide whether they should be reverted or what the direction that @ljharb wants to take this library in. The problem therein being that technically this is not bug and the change probably should have been in the breaking changes list.

---

Ok, moving on to your example. Lets break this down and simplify it a bit. The problem you are experiencing can be attributed in its entirely to the change made in #2396 (see above). Again, this means that paths within the same group are sorted alphabetically by their path segments. Given that your configured groups array does not contain either parent or sibling, all of the parent and sibling imports are added to the unknown group and are therefore all part of the same group.

Therefore, your test case can be simplified to the following:

test({
    code: `
        import { TypeBindingFixtures } from '../fixtures/TypeBindingFixtures';
        import { BindingService } from './BindingService';
        import { BindingServiceImplementation } from './BindingServiceImplementation';
    `,
    options: [{
        alphabetize: {
            order: 'asc',
            caseInsensitive: true,
        },
        groups: ['builtin', 'external'],
        'newlines-between': 'always',
    }],
}),

According to my explanation above, the error you are receiving is not bug. Technically, when paths are sorted alphabetically by path segment, any path that starts with a single . (ie, a sibling import) will be sorted above a.. (a parent import). Since all of these parent and sibling imports are part of the unknown group, the plugin is under no obligation to sort these in the parent > sibling order.

Ok, so how do we fix this? Essentially, this PR adds and option allowing you to control the order in which imports are sorted. This would mean an eslint configuration change. Your current config would need 2 changes:

1. Add the intraGroupOrdering option and set it to true.
2. Add a nested subgroup in your groups array that places parent imports above sibling imports

Therefore your resulting configuration would look something like this:

options: [{
    alphabetize: {
        order: 'asc',
        caseInsensitive: true,
    },
    intraGroupOrdering: true,
    groups: ['builtin', 'external', ['parent', 'sibling']],
    'newlines-between': 'always',
}],

If you still want all of the potentially unknown imports to be sorted together (no newlines), you could potentially add all of those remaining import group types to the final subgroup (given that we already know what all of them are).

So something like this:

intraGroupOrdering: true,
groups: ['builtin', 'external', ['internal', 'parent', 'sibling', 'index', 'object', 'type', 'unknown']],

---

But this error is a regression. Until eslint-plugin-import version 2.26.0, a grandparent import was before a parent import. Is there a reason to change this behavior? Is that the expected behavior? If so, should we update docs and changelog according to this behavior? This way any user would know it's not a bug but the new expected behavior.

To answer this final part, yes this is a "change" but not necessarily a "regression". I personally believe that the method that this rule uses for alphabetization should be documented and that that change could potentially be part of this PR. I also understand that having to specify every import type in the groups array (like in the final example I gave above) to essentially have the same behavior as v2.26 and below is not ideal. But ultimately, I think most people are more likely to be update their configuration and accept that there will be import ordering changes in their files when they do.

[notaphplover]

@Pearce-Ropion Thank you so much! Ok now I got the context everything is clear to me. As soon as it's an intended change I can afford the changes on my source code.

The changes made in #2396 were not made by me and therefore is not up to me to decide whether they should be reverted or what the direction that @ljharb wants to take this library in. The problem therein being that technically this is not bug and the change probably should have been in the breaking changes list.

I totally respect the change, just consider documenting it as BC, for me that was the issue: from a day to another the CI tried to update the linter, which resulted complaining about tons of errors in the imports order. Then I went to the changelog and no BC was documented, it was a normal minor release.

I can understand the argument this is a fix and not a regression, but those subtelties are hard to acknowledge without coming here for help since most of us do not know the design decissions taken here in the library. For that reason maybe, as you commented, it would be more practical to warn (a BC sections feels great) about these changes, even if they are technically fixes. The hint given in the changelog was:

order: move nested imports closer to main import entry (#2396, thanks @pri1311)

Which maybe lacks a little bit of detail. Sorry to be picky, just doing my best to give you my honest feedback looking forward to improve things in the future. You guys are great and I really appreciate all the work you are doing! ❤️

[rgant]

Any chance this is going to be merged?

[tjx666]

still stay "eslint-plugin-import": "2.26.0" wait for this pr to be merged

[tonypine]

Looking forward to this ❤️ , thanks for the PR

[ljharb]

@Pearce-Ropion if the original change is considered breaking, then the right approach is to revert it, and instead of provide it behind an option. Can you rebase this PR and update it to achieve that?

[akx]

Also looking forward to this (so as to avoid ✖ 745 problems (745 errors, 0 warnings) when upgrading from 2.26.0.

Is there anything I can do to move this forward?

[rgant]

@Pearce-Ropion if the original change is considered breaking, then the right approach is to revert it, and instead of provide it behind an option. Can you rebase this PR and update it to achieve that?

This change has been live for so long is that still the correct way to handle this?

[Pearce-Ropion]

Sorry all, I've been very busy at work and haven't had time to work on this.

At this point, I'm inclined to agree with @rgant on this one. Reverting the thing that caused this issue will likely also be a breaking change for everyone who started using this rule since v2.27 released.

I haven't tested it, but I'm fairly sure my changes will still work with or without reverting the initial issue.

[brendonrapp]

I would love to stop being locked at 2.26.0. Any motion on this?

[jraoult]

I would love to stop being locked at 2.26.0. Any motion on this?

I've been following this (and other related issues) for a while @brendonrapp, but I ended up delegating the sorting part to Perfectionist so I could upgrade eslint-plugin-import while being able to define my sorting preference exhaustively. It's working well for me and might work for you:

"perfectionist/sort-imports": [
      "error",
      {
        groups: [
          ["external", "builtin"],
          ["parent", "sibling"],
        ],
        "newlines-between": "always",
      },
    ],

[ljharb]

Ideally, I'd be able to add a new option that matches the post-2.26 behavior, and separately revert the default behavior to the pre-2.26 behavior.

that way, the breaking change is reverted, but all the users that will break have an easy fix.

My hope is that this PR can be made to do that.

[cipchk]

Any progressing?