Hi@Pearce-Ropion, thank you so much for the PR. Just a little suggestion: would you mind not using theResolves 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 theoryhttps://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.

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!)

Hi@Pearce-Ropion, thank you so much for the PR. Just a little suggestion: would you mind not using theResolves 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.

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. Untileslint-plugin-import version2.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.

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 sortparent imports beforesibling 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 configuredgroups array does not contain eitherparent orsibling, all of theparent andsibling imports are added to theunknown 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, asibling import) will be sorted above a.. (aparent import). Since all of theseparent andsibling imports are part of theunknown group, the plugin is under no obligation to sort these in theparent > 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 theintraGroupOrdering option and set it totrue.
2. Add a nested subgroup in yourgroups array that placesparent imports abovesibling 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 potentiallyunknown 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. Untileslint-plugin-import version2.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 thegroups 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.

@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! ❤️

Any chance this is going to be merged?

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

Looking forward to this ❤️ , thanks for the PR

@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?

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?

@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?

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.

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

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 toPerfectionist so I could upgradeeslint-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",},],

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.

Any progressing?