Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Oct 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ module.exports = {
},
],
pathGroupsExcludedImportTypes: ['react', 'react-dom', 'prop-types'],
alphabetize: {
order: 'asc',
},
Copy link
Collaborator

@dominicfraser dominicfraser Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',? 🤔

Copy link
Member Author

@olliecurtis olliecurtis Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
],

Expand Down
14 changes: 14 additions & 0 deletions test/order-fail.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/* eslint-disable no-unused-vars,import/no-unresolved,import/extensions */
// import/order
import ReactDom from 'react-dom';

Check failure on line 3 in test/order-fail.jsx

View workflow job for this annotation

GitHub Actions / Run-Tests / Test

`react-dom` import should occur after import of `react`
import PropTypes from 'prop-types';
import { Component, Fragment } from 'react';

import { fontSizeSm } from '@skyscanner/bpk-foundations-web/tokens/base.es6';

Check failure on line 7 in test/order-fail.jsx

View workflow job for this annotation

GitHub Actions / Run-Tests / Test

`@skyscanner/bpk-foundations-web/tokens/base.es6` import should occur after import of `@skyscanner/backpack-web/bpk-component-link`
import ArrowUpIcon from '@skyscanner/backpack-web/bpk-component-icon/sm/long-arrow-up';
import BpkButton from '@skyscanner/backpack-web/bpk-component-button';
import BpkButtonLink from '@skyscanner/backpack-web/bpk-component-link';

import Component1 from '../Component1';
import Component2 from '../../Component2';
/* eslint-enable no-unused-vars,import/no-unresolved,import/extensions */
3 changes: 2 additions & 1 deletion test/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"test:fail-bpk": "echo 'Expecting failure' && eslint bpk-token-fail.jsx; if [ $? -eq 1 ]; then exit 0; fi",
"test:fail-prettier": "echo 'Expecting failure' && eslint prettier-fail.jsx; if [ $? -eq 1 ]; then exit 0; fi",
"test:fail-react": "echo 'Expecting failure' && eslint react-fail.tsx; if [ $? -eq 1 ]; then exit 0; fi",
"test": "npm run test:pass && npm run test:fail-jsdoc && npm run test:fail-bpk && npm run test:fail-prettier && npm run test:fail-react"
"test:fail-import-order": "echo 'Expecting failure' && eslint order-fail.jsx; if [ $? -eq 1 ]; then exit 0; fi",
"test": "npm run test:pass && npm run test:fail-jsdoc && npm run test:fail-bpk && npm run test:fail-prettier && npm run test:fail-react && npm run test:fail-import-order"
},
"dependencies": {
"prop-types": "^15.5.10",
Expand Down
6 changes: 3 additions & 3 deletions test/pass.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import ReactDom from 'react-dom';

import ExternalLibrary from 'external-library';

import { fontSizeSm } from '@skyscanner/bpk-foundations-web/tokens/base.es6';
import BpkButton from 'bpk-component-button';
import ArrowUpIcon from 'bpk-component-icon/sm/long-arrow-up';
import BpkButton from '@skyscanner/backpack-web/bpk-component-button';
import ArrowUpIcon from '@skyscanner/backpack-web/bpk-component-icon/sm/long-arrow-up';
import BpkButtonLink from '@skyscanner/backpack-web/bpk-component-link';
import { fontSizeSm } from '@skyscanner/bpk-foundations-web/tokens/base.es6';

import SomeCommonFunctionality from 'common/some-functionality';

Expand Down
Loading