Skip to content

Chore: Configure ESLint import order rules#7

Merged
gvjacob merged 8 commits into
mainfrom
chore/eslint-import
Jan 18, 2023
Merged

Chore: Configure ESLint import order rules#7
gvjacob merged 8 commits into
mainfrom
chore/eslint-import

Conversation

@gvjacob
Copy link
Copy Markdown
Member

@gvjacob gvjacob commented Jan 18, 2023

What This Does

  • Install eslint-plugin-import and its Typescript resolver
  • Configure import/order rule based on our common use cases (more details in threads below!)
  • Fix our files based on the ne w rule
  • Also, make sure type imports are annotated with type

How to Test

  1. Review the changes in this PR
  2. Or, go to a .ts file and tinker around with different imports and see how the ESLint linter warns you

@vercel
Copy link
Copy Markdown

vercel Bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
supernoodle-prototype ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 18, 2023 at 3:22PM (UTC)

Comment thread .eslintrc.json
"import/order": [
"warn",
{
"groups": ["builtin", "external", "internal", "type"],
Copy link
Copy Markdown
Member Author

@gvjacob gvjacob Jan 18, 2023

Choose a reason for hiding this comment

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

In order:

builtin: any built in JS modules like fs
external: installed packages
internal: local code
type: TS type declarations

Everything else not classified under these groups will be put last. You can read more about the config here. There are some other options but I don't think we're currently using them so I want to wait until we do before we configure them.

Comment thread .eslintrc.json
"warn",
{
"groups": ["builtin", "external", "internal", "type"],
"newlines-between": "always",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Make sure there's a newline between import groups

Comment thread .eslintrc.json
Comment on lines +40 to +43
"alphabetize": {
"order": "asc",
"caseInsensitive": true
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sort imports within each group. This make sure for example that @src imports always come after @public

Personally don't see a compelling need to be opinionated on sorting within each group so alphabet order seems the best choice to me.

@gvjacob gvjacob marked this pull request as ready for review January 18, 2023 11:12
@gvjacob gvjacob requested a review from joshpensky January 18, 2023 11:14
@gvjacob gvjacob self-assigned this Jan 18, 2023
@gvjacob gvjacob added this to the 0.1.0 milestone Jan 18, 2023
@gvjacob gvjacob changed the title Chore: Configure ESLint import rules Chore: Configure ESLint import orer rules Jan 18, 2023
@gvjacob gvjacob changed the title Chore: Configure ESLint import orer rules Chore: Configure ESLint import order rules Jan 18, 2023
Copy link
Copy Markdown
Contributor

@joshpensky joshpensky left a comment

Choose a reason for hiding this comment

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

Thanks for adding it! 🏁 Just a few non-blocking comments (based on other projects I've used this with)

Comment thread .eslintrc.json Outdated
"global-require": 0,
"import/prefer-default-export": 0,
"import/order": [
"warn",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

personally wouldn't mind this being an error 😉

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fine by me!

Comment thread .eslintrc.json
"rules": {
"global-require": 0,
"import/prefer-default-export": 0,
"import/order": [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion for another rule (take it or leave it!) that ensures react and next are always the first import for the external group

        "pathGroups": [
          {
            "pattern": "react",
            "group": "external",
            "position": "before"
          },
          {
            "pattern": "next",
            "group": "external",
            "position": "before"
          },
          {
            "pattern": "next/**",
            "group": "external",
            "position": "before"
          }
        ],

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't mind this, although unsure why it's not working at all when I added this 🤔 It was also clashing with type imports from next/app. I'm going to table this for now but good note nonetheless!

@gvjacob gvjacob merged commit 7d939fd into main Jan 18, 2023
@gvjacob gvjacob deleted the chore/eslint-import branch January 18, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants