[Fix] Revert breaking group change in order

[yndajas]

1fa2971 changed the way groups work when there is only one, leading to single nested groups being treated as though they were unnested. Prior to this, if you wanted to group e.g. builtin and external imports together at the top and everything else together as a second group, a single nested group was the way to do it

It appears that this change was unintentional, and was made to try to fix what seems to be a misunderstanding around nested groups ([#2687]). The docs continue to suggest that nested groups should be "mingled together" and makes no reference to a single nested group with no other groups being an invalid option

This therefore reverts the change to how groups work when there is only one. No documentation change should be necessary given this is already the described behaviour

Surfaced in ministryofjustice/hmpps-accredited-programmes-ui#129
Closes #2687

[ljharb]

To be clear, I think order should matter in nested groups, but that would be a breaking change, so this PR is the right approach.

[yndajas]

@ljharb any idea why this fails in one (or more) environments? Is this a common issue unrelated to this PR?

[ljharb]

@yndajas the windows failures are expected; the node failure was a fluke, and I've rerun it.

[ljharb]

I'll land this once we get more travis-ci credits.

[jraoult]

I've been holding of any upgrade since 2.26.0. After that release, the change in ordering algorithm introduced a regression. My understanding was that this PR was intended to reverse the sorting behaviour to got back to the one in 2.26.0. If it was the intention, I can report this is still not right then.

The following configuration:

"import/order": [
      "error",
      {
        alphabetize: {
          order: "asc",
          caseInsensitive: true,
        },
        groups: [
          ["external", "builtin"],
          ["parent", "sibling"],
        ],
        "newlines-between": "always",
      },
    ],

pre 2.27 was passing for:

import {parent} from "../a.js";
import {sibling} from "./a.js";

but from 2.27 (including 2.28.1) only accepts:

import {sibling} from "./a.js";
import {parent} from "../a.js";

I understand that the nested group order is "unspecified" in the doc but it is still a change of behaviour from 2.26. So should I update my code to match the new order or is it still a regression waiting for a fix?

[yndajas]

@jraoult are you expecting the former based on ../ coming before ./? In my experience ./ gets sorted before ../, so the latter example would be what I'd expect when parent and sibling imports are intermingled by being in the same nested group

[jraoult]

@jraoult are you expecting the former based on ../ coming before ./? In my experience ./ gets sorted before ../, so the latter example would be what I'd expect when parent and sibling imports are intermingled by being in the same nested group

@yndajas I'd expect the nested group to follow the order of the array. So if the nested group conf is ["parent", "sibling"] I'd expect ../ before ./ but I know this is not how it is documented right now (cf. #2854 (comment)) or supposed to work and out of the scope of this PR.

So at minima, I'd expect the order to not change in a minor version. Meaning that 2.28 should order import the same way as 2.26 for the same given configuration. It seemed to be the aim of this PR to revert to the behaviour of 2.26 but from my tests failed to do so.

So my real question is: should I consider this new way of sorting the nested groups a feature, in which case I'll update my code base to match it, or a bug in which case I'll wait until it is fixed.

[yndajas]

@jraoult so in 2.26 (before the breaking change in 2.28.0) the order in nested groups was meaningful? In my (limited) experience with this package the order within nested groups didn't matter (per the docs) until 2.28.0, in which it mattered only when there was just one nested group*, but I'm quite late to the game here

* the way I got around this in 2.28.0 was by adding another nested group to make it not matter again

This is probably one for @ljharb in any case!

[jraoult]

@yndajas I don't have any particular opinion on the order TBH. I just expect the import order to stay the same from minor version to minor version so my CI doesn't break. If it is decided that this new way of ordering is stable and the way forward I'm happy to do a one off run on the codebase and call it a day but my gut feeling is that it should have been a major version then 🤷‍♂️

[ljharb]

Indeed it's a breaking change if the semantics of nested or unnested groups changed within v2.

It sounds like there's still a bit of regression left; i'd like anything that passed in v2.26 to pass in v2.28 (with the same options, of course). @yndajas got another PR in you? :-D

[yndajas]

Indeed it's a breaking change if the semantics of nested or unnested groups changed within v2.

It sounds like there's still a bit of regression left; i'd like anything that passed in v2.26 to pass in v2.28 (with the same options, of course). @yndajas got another PR in you? :-D

Happy to work on a fix if one is needed, but I'm still a little unclear on what the difference between 2.26 and 2.28.1 is for this rule 🤔 I haven't tested 2.28.1 on one of my own codebases, but pre-2.28.0 I'd have expected ./ to sort before ../ within the same group if sorting alphabetically

Based on release dates I've probably only used 2.27.5 (started on the project that's using this package in April), so I might be missing some context here! This PR was likely focused on fixing a regression specific to 2.28.0 (versus 2.27.5, bringing it back in line with the docs)

I can take a look next week to see if 2.28.1 is working as I'd expect in the project I'm currently on

---

Re: within-group ordering, can you not de-nest the group? If order within nested groups matters, what's the use of nested groups? Surely the whole purpose of nesting is to identify a group of the same rank?

E.g. if you want type C imports before type D imports, instead of:

[
  [A, B],
  [C, D]
]

could you not just write this?

[
  [A, B],
  C,
  D
]

[yndajas]

My findings on this in the project I'm working on are a little inconclusive: we seem to have dropped back to 2.27.5 through a recent Renovate PR, and updating to 2.28.0 again and reverting to a single nested group no longer appears to be problematic. In any case, everything is (also) fine after upgrading to 2.28.1 with a single nested group, so hopefully it's fixed (if it even was broken - I'm more confused now than ever 🤷🏻‍♀️)

More detail here: ministryofjustice/hmpps-accredited-programmes-ui@47bf31b

[jraoult]

FYI @ljharb @yndajas, the regression is still here in my case in the freshly released 2.29.0, so I'm staying in 2.26.0 for now but not too sure about the path forward.

[ljharb]

@jraoult can you file a new issue allowing me to repro the problem? Staying on v2.26 is definitely not a viable option long term.

[yndajas]

@jraoult does this definitely not work for you?

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

[jraoult]

@ljharb @yndajas so my configuration is:

 "import/order": [
      "error",
      {
        alphabetize: {
          order: "asc",
          caseInsensitive: true,
        },
        groups: [
          ["external", "builtin"],
          ["parent", "sibling"],
        ],
        "newlines-between": "always",
      },
    ],

and typically, this used to pass in 2.26:

import { isNotNull } from "option-t/esm/Nullable";

import { combineNullables } from "../util/combineNullables.js";
import type { BigFraction } from "./BigFraction.js";
import { FRACTION_ZERO } from "./fraction.js";

but fails after 2.26 with message: ../util/combineNullables.js import should occur after import of ./fraction.js

[yndajas]

@jraoult Yeah that's expected behaviour based on the docs (if I understand them correctly), since the path comes later under alphabetical sorting and you've got parent and sibling imports at the same rank by nesting them

I'm not sure sure why it worked the other way in 2.26.0. As I've suggested above, denesting should allow you to require parent imports to come before sibling imports, which should be a single config change and not require any other changes throughout your codebase, all being well!

As @ljharb said, it's probably best to open a separate issue if that doesn't solve the problem

[jraoult]

@ljharb issue filled here: #2909

[jraoult]

denesting should allow you to require parent imports to come before sibling imports

@yndajas, but then I can not leverage newlines-between the same way.

[yndajas]

@yndajas, but then I can not leverage newlines-between the same way.

Ahh I see what you mean. Might be good to add that to the issue, because that clarifies things a lot