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

tests(eslint): add import/order rule #12998

Merged
merged 8 commits into from
Sep 2, 2021
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Aug 31, 2021

https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md

This adds some tooling to apply the implicit ordering rule we've been using: order imports by builtin/external/local modules. Additionally, I enabled an alpha sort within each group. This plugin works for esm and commonjs, but the latter isn't always autofixable so I've only applied this rule to folders that have been converted to esm.

There are a few more options to choose from. Would we like to add a newline between the different import groups?

@connorjclark connorjclark requested a review from a team as a code owner August 31, 2021 21:20
@connorjclark connorjclark requested review from adamraine and removed request for a team August 31, 2021 21:20
@google-cla google-cla bot added the cla: yes label Aug 31, 2021
@patrickhulce
Copy link
Collaborator

Would we like to add a newline between the different import groups?

No strong preference, but I do in my other projects.

One other note: we're going to need to be more careful about this when we upgrade our core tests as many of those use mock function invocations before specific requires that are not hoisted by jest by default.

@brendankenny
Copy link
Member

brendankenny commented Aug 31, 2021

I can't say import/require order has ever bothered me very often, so i'm not relishing an eslint --fix after every time I make a change to them. Other places I've committed code with sorted imports it can sometimes be annoying (especially with IDE support for immediately showing lint errors).

Maybe without the alphabetization it wouldn't be too bad since, as you said, we generally follow that order? Splitting up parent/sibling imports also seems like overkill to me, so maybe it makes sense to merge those instead of following the default.

@patrickhulce
Copy link
Collaborator

Maybe without the alphabetization it wouldn't be too bad

I'm also loose +1 to no alphabetization, 100% of the changes in this PR that I look at and go "pft, c'mon" were the alphabetized ones :)

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

I remember alphabetical imports being annoying, but if I can configure VScode where I never have to think about it then I think it's fine. Maybe this extensions, not sure if that will obey the groups though.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 31, 2021

You ought to be able to configure VS Code to apply eslint autofix on save. That works well for me (except for projects that have a lint rule to auto-delete unused code... that will randomly cause you to lose code mid refactor!)

re: alpha, I always prefer when there is only one right way to write code, as that makes merge conflicts less frequent. two separate PRs add a new import, but if they can choose the order randomly then a conflict will occur for someone. a slightly-contrived case... in general I won't be able to stop my self from adding imports in not-alpha order so a tool that does it for me is nice :)

@brendankenny
Copy link
Member

You ought to be able to configure VS Code

this seems like not enough bang for one's buck. I don't remember the last time this came up even as nits in people's reviews. Paving cowpaths (like automating builtin/external/internal ordering) is fine/can be good, but fixes like this and this seem more like ordering for the sake of ordering than fixing any readability or maintainability issue.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Sep 1, 2021

fwiw, IIRC paul is also team-alphasort. so that's 2v2v1 here :) (I'm marking adam as neutral...)

Can we land if I undo the alphasort (and redo the initial lint), keeping the grouping+newlines?

I don't remember the last time this came up even as nits in people's reviews.

Yes, it's extra nitty to point out import orders, which is why I never do, esp. since I'm aware there is automated tooling that could be doing it for us. so even though this is where my preference lies I refrain from commenting on it (as the problem is really tooling, not from the author's PR).

I just value consistency with zero effort, and being able to Ctrl+s to enforce it is nice shrug

@patrickhulce
Copy link
Collaborator

Can we land if I undo the alphasort (and redo the initial lint), keeping the grouping+newlines?

Yeah for sure 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants