import/order relative import sorting order changed in 2.27.0

[remcohaszing]

Given the following rule config:

module.exports.rules = {
  'import/order': [
    'error',
    {
      alphabetize: { order: 'asc', caseInsensitive: true },
      groups: ['builtin', 'external', 'internal', ['parent', 'sibling', 'index'], 'object'],
      'newlines-between': 'always',
      warnOnUnassignedImports: true,
    }
  ]
}

In eslint-plugin-import 2.26, sibling imports were sorted after parent imports. In versions 2.27.0 to 2.27.5, siblings are sorted before parents.

Here’s an example of previously valid, but now invalid code:

import { fooz } from '../baz.js'
import { foo } from './bar.js'

[remcohaszing]

I think this is also what @cojoclaudiu meant with #2669 (comment), but it appears to be unrelated to that issue.

[btecu]

Also having issues with the import/order getting:
error `@ember-data/model` import should occur after import of `@ember/template` import/order

[ljharb]

Indeed, this seems to be broken. cc @Pearce-Ropion

[Twipped]

Definitely should have been a breaking change, at a minimum.

[Pearce-Ropion]

Given that its a bug, I don't think a breaking change would have made sense since it wasn't intentional.

[ljharb]

@Twipped indeed, it's only a breaking change if it's intentional (and i'm not sure what would be larger than "breaking change")

[Pearce-Ropion]

Ok, I've confirmed that this is caused by the alphabetical sortingFn introduced in 347d78b. The sorter is sorting parent imports after siblings because ../ is sorted after ./ alphabetically. Currently the sorting fn has no knowledge of nested groups, it will just sort all elements that are in the same group alphabetically.

@ljharb What is the expected behavior for nested groups? The docs aren't very clear.
https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md#groups-array

In the description for groups it says:

The enforced order is the same as the order of each element in a group.

But in the example given for groups it says that nested groups can be mingled together

> ['sibling', 'parent'], // Then sibling and parent types. They can be mingled together

And then there is nothing in the alphabetize description that relates to the sorting of nested groups.

---

My assumption is that we should sort nested groups in element order and then alphabetically. If this is the case, the alphabetical mutations will need to be aware of the group order which will make that code much more complicated.

Alternatively, we can change how the initial ranks are set by increasing the rank within nested groups in addition to just between groups.

Thoughts?

[ljharb]

I think that the primary thing is to avoid the regression - so the default behavior should remain "whatever it did before".

However, if the ideal behavior we decide on is different, we should also provide an option for that behavior.

I would assume that ['sibling', 'parent'] means that neither one should have any preference. If they want siblings sorted before parents, they'd do 'sibling', 'parent' without the array.

[Pearce-Ropion]

I would assume that ['sibling', 'parent'] means that neither one should have any preference. If they want siblings sorted before parents, they'd do 'sibling', 'parent' without the array.

Unless you want sibling to be sorted before parent without newlines.

[ljharb]

I believe that's what the distinctGroup option is for?

[jraoult]

Like @Pearce-Ropion, I've been relying on the fact that nested groups respect the array order and
then set "newlines-between": "always" to get a split between them.

I believe that's what the distinctGroup option is for?

The doc seems to imply that distincGroup is for pathGroups only (cf. https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md#distinctgroup-boolean). So are you saying that to get stable ordering in a nested group and newlines between groups, we should use pathGroups instead of groups?

[ljharb]

ah, fair point. We don't seem to have a way to avoid newlines between groups.

or, rather - the only way we currently have to signal "treat these two groups differently" is putting them in an array. One thing that could imply is "don't newline between them, but keep their relative ordering", and another thing that could imply is "treat all groups in the array as being of equal rank".

It can't imply both, so which one should it imply?

The enforced order is the same as the order of each element in a group

This in the docs implies to me that it should be the former. If that's what previous versions of the plugin in v2 do, then I think that's what we should go with - and if someone wants the intermingling, then we'd need a new feature request filed for that, after this bugfix is done.

[Pearce-Ropion]

So in order to fix the regression, we would essentially need to revert 347d78b.

The following uses the example ../barz.js and ./bar.js:

The problem is that the new sorter (v2.27+) splits sorting of import paths by the / character (added in 347d78b). This results in an attempt to sort .. and . instead of the full path. This results in ./bar.js being placed before ../barz.js.

In 2.26 and below, we attempt to sort on the the full import path. Thus ../barz.js and ./bar.js results in ../barz.js being placed before ./bar.js.

---

It is worth noting that before the "alphabetization" takes place, both the parent and sibling imports have the same rank. So in both v2.26 and v2.27+, the ordering of the groups in the nested array has no affect on the final order of the imports, just that they are placed together without new lines.

Instead of reverting, one option could be adding a case that specifically targets parent imports and places them before sibling imports. The question would then be, what if the nested group array had sibling placed first (ie, ['sibling', 'parent', 'index'],)? Should we only do this if parent specifically comes before sibling?

---

So in my mind, a real fix would be to change the alphabetical sorter to sort based on the original groups configuration option and not just the ranks.

[ljharb]

Seems like you have the real fix in mind :-) a PR would be appreciated!

[Pearce-Ropion]

So the only issue with the proposed fix is that it will probably change the sorting for many people across the board since it won't just affect the sorting in this parent/sibling case. It will change how alphabetical sorting works for every nested group which may constitute a minor or major version change.

[ljharb]

hmm. i think the first thing would be to address the regression without breaking any other tests.

if it's going to drastically change the ordering then it's probably a breaking change and it'd have to sit for awhile.

[Pearce-Ropion]

I've put up #2721. I think it needs more unit testing but figured I'd get the PR out there

[mihkeleidast]

I've filed #2885, which should fix the regression, while keeping everything else working as-is.

[alex-tsx]

Low-cost way to deal with this was to change this setting

  "groups": [["external", "builtin"], "internal", ["parent", "sibling", "index"]],

into:

  "groups": [["external", "builtin"], "internal", "parent", "sibling", "index"],

Then you will only need to insert empty lines between parent & sibling import groups in your codebase (low-cost part).

And I highly recommend doing this, if you rely on import/no-cycle rule. Bumping up from v2.26 to v2.29 reduced ESLint checks for us by ~ 6 minutes !