[ARGG-741]: Add rule to set alphabetical order of imports

[olliecurtis]

There is an issue from the mini-css-extract-plugin where when import order is mixed about, and causes CSS issues, so this PR looks to enforce a consistant import structure across all components to avoid the issue.

Example error

Failed to compile.

(undefined) chunk 1 [mini-css-extract-plugin]
Conflicting order. Following module has been added:
 * css ./node_modules/css-loader/dist/cjs.js??ref--5-oneOf-7-1!./node_modules/postcss-loader/src??postcss!./node_modules/@skyscanner/backpack-web/bpk-component-button/src/BpkButtonV2/BpkButton.module.css
despite it was not able to fulfill desired ordering with these modules:
 * css ./node_modules/css-loader/dist/cjs.js??ref--5-oneOf-7-1!./node_modules/postcss-loader/src??postcss!./node_modules/@skyscanner/backpack-web/bpk-component-text/src/BpkText.module.css
   - couldn't fulfill desired order of chunk group(s) component-x, component-y
   - while fulfilling desired order of chunk group(s) components-z

This change is autofixable as well with the --fix flag so means it shouldn't come as a too hard to fix and not considered 'breaking'

[dominicfraser]

This change is autofixable as well with the --fix flag so means it shouldn't come as a too hard to fix and not considered 'breaking'

I'm not against this conceptually - but I do remember feedback we got the last time we adjusted import order rules.

It was very disruptive for people for two reasons:

• Volume of files changed when fixing
• When import order moved around any inline comments, eslint suppressions, or typescript suppressions did not move with them, meaning that most people did need manual work beyond just --fix to make a PR pass

So while the exact rule itself isn't breaking, its possible that the side effects of meeting it would count as such? I can argue either way here, just one to consider.

[dominicfraser]

If we do decide on a breaking release, it's worth considering what else to put in it:

• 'import/prefer-default-export': 'off', -- turning this off, since the large codebases internally override this, and some invert it to add 'import/no-default-export': 'error',
• eslint-plugin-prettier v9 -- I haven't checked if this is breaking for us yet or not

[anambl]

My only concern with making it major is that people might feel more inclined to postpone the update, I think since there are quick fixes we can make it minor, but don't feel very strongly about either option 😄
How is it related to 'import/prefer-default-export': 'off' and 'import/no-default-export': 'error',? 🤔

[olliecurtis]

Hey folks thanks for the review, understand that this makes big changes across large codebases and this was something that I raised within the web enablement team around just do it locally or benefit everyone.

Whilst it would make a big change it as mentioned is auto fixable so whilst looks disruptive isn't a heavy hitter, but would require a little checking in case it did have an effect or actually fixes warnings folks have ignored and turned off in webpack for css ordering.

I would avoid adding more rules and scope to this ticket because it's not an impact and or might cause impact to the result of the current work, which is currently based on just this

[dominicfraser]

Not for this current ticket, but if we decide to release this as a major then we should pause to think what else to include in the major (maybe nothing).

To argue for a major: The side effects of moving imports around are not autofixable.

If someone has:

// @ts-expect-error 
import { getSeoComponentLinks } from './seoComponentLinks';
import { getDestinationTravelEntityId } from './destinationTravelEntityId';

and the imports move, the comment will not.

We got very poor feedback from squads the last time we released an import order change - people definitely considered it a major. They definitely considered it a heavy hitter, having to manually update 100s of suppressions inside import groups.

I do not think we should ever shy away from a major to effectively hide how much work something would be - if we think it is, then it is.

To argue against a major:

The lint rule itself is autofixable, so not major. From e-c-s's point of view side effects are out of scope for determining the semver of a change.

[anambl]

I do not think we should ever shy away from a major to effectively hide how much work something would be - if we think it is, then it is.

💯 But I believe for most projects this will not be a big change worthy of a major release and those projects will be postponing the update seeing it as a potential time consuming task. I might be wrong though actually given we already have feedback from squads seeing this as a major 😅 not sure how long ago that was and if it still applies.
I think we tested this already in a project (can't remember where), what was the impact on that project?

From e-c-s's point of view side effects are out of scope for determining the semver of a change.

I agree with this. Also completely agree that the side effects can have a big impact making it a bigger task, but I'm not too sure how many projects will be impacted by that.

[dominicfraser]

Unknown! And can change at any time.

I think we decide conceptually holistically - then apply that thinking, rather than in this case trying to guess which projects - there are non-Skyscanner projects too, and while they're not our primary customers they do get impacted.

I'm happy with either (can really argue either side to be devil's advocate) - just wanted to make sure we considered it, since last time it did make people unhappy.

However while its worth taking that into account, when it is a change that fixes a problem then its not at all to say not to do it, just to make sure the semver matches whichever mindset from the two (or more) arguments above.