Skip to content

eslint: Use consistent-type-imports #872

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

Closed
wants to merge 1 commit into from

Conversation

auscompgeek
Copy link
Member

@auscompgeek auscompgeek commented Jul 17, 2022

esbuild will parse and ignore type imports, so let's actually give it this information.

@typescript-eslint/consistent-type-imports

Ref: #714

Checklist

@auscompgeek auscompgeek requested a review from pokey as a code owner July 17, 2022 07:02
@pokey
Copy link
Member

pokey commented Jul 17, 2022

Seems fine, but a couple questions:

  • Will VSCode automatically start using this style when it does an auto-import for us, both via
    • autocomplete, and
    • quick fix?
  • Also, will pre-commit automatically fix this for us? I can't remember if we have eslint --fix set up

@auscompgeek
Copy link
Member Author

This is ESLint autofixable, so will show up as a quick fix in VSCode. I'd love to know how to make autoimport do the right thing though.

We currently don't have pre-commit running ESLint, I recall you had concerns around pulling in the relevant plugin versions?

@pokey
Copy link
Member

pokey commented Jul 17, 2022

This is ESLint autofixable, so will show up as a quick fix in VSCode.

Ah no I don't mean once it's already imported. I mean if you have a symbol that you haven't yet imported and you run quick fix to do the auto import

I'd love to know how to make autoimport do the right thing though.

Does it not? It might be smart enough to use type if we have this eslint config

We currently don't have pre-commit running ESLint, I recall you had concerns around pulling in the relevant plugin versions?

Ah right yeah I remember now. I think the issue was we had nothing to auto fix so didn't seem worth it. Maybe we should revisit now we have more auto fixers?

@pokey
Copy link
Member

pokey commented Oct 1, 2022

@auscompgeek I'm curious why you closed this one. I'm overall supportive of this direction, modulo the questions above

@auscompgeek
Copy link
Member Author

I'm not really sure the questions are addressable in any straightforward manner. There's also (unsurprisingly) a lot of merge conflicts by this point, so needs a rebase.

Happy for someone else to tackle this, but this probably doesn't win too much for us right now.

@pokey
Copy link
Member

pokey commented Oct 2, 2022

I think basically it would involve just pulling this branch, then trying to do an auto-import and see what happens. But I agree this rule probably isn't a massive win for us, so probably not super high priority

@pokey pokey deleted the auscompgeek/eslint-type-imports branch October 2, 2022 16:05
@pokey pokey mentioned this pull request Oct 2, 2022
8 tasks
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.

2 participants