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

Standardize on type-only imports #32368

Closed
roryabraham opened this issue Dec 1, 2023 · 10 comments
Closed

Standardize on type-only imports #32368

roryabraham opened this issue Dec 1, 2023 · 10 comments
Assignees
Labels

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Dec 1, 2023

Slack context: https://expensify.slack.com/archives/C01GTK53T8Q/p1701371123201489

Problem

We are inconsistent about how we import types. Type-only imports are preferred because they disappear at runtime, making the program more lightweight. It also improves code clarity because it's obvious that a type is being imported and not some other value.

Solution

Enable the consistent-type-imports lint rule w/ prefer: type-imports

Issue OwnerCurrent Issue Owner: @chrispader
@roryabraham roryabraham self-assigned this Dec 1, 2023
@roryabraham
Copy link
Contributor Author

cc @blazejkustra @fabioh8010 @chrispader can one of you comment for assignment please?

@chrispader
Copy link
Contributor

@blazejkustra you wanted to take this one? Otherwise i can also open a PR :)

@blazejkustra
Copy link
Contributor

@chrispader Sure, you can take it 😄

@chrispader
Copy link
Contributor

Created the PR here: #32475

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels Dec 5, 2023
@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Dec 29, 2023
Copy link

melvin-bot bot commented Dec 29, 2023

This issue has not been updated in over 15 days. @chrispader, @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@roryabraham
Copy link
Contributor Author

We're waiting on an upstream PR from @chrispader here. This isn't very high priority, so I'm not opposed to just closing this out for now so it's not a distraction. Up to you @chrispader

@roryabraham roryabraham removed the Reviewing Has a PR in review label Dec 29, 2023
@chrispader
Copy link
Contributor

Starting to work on this now! Got some spare cycles..

cc @roryabraham @blazejkustra @fabioh8010

@chrispader
Copy link
Contributor

chrispader commented Dec 30, 2023

Turns out, we don't need an upstream change 🎉 There's a rule for enforcing type specifier style:

https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/consistent-type-specifier-style.md

PR updated and ready for review 🚀

@chrispader
Copy link
Contributor

@roryabraham i think we can merge this one :)

@roryabraham
Copy link
Contributor Author

@chrispader what one? 😄 I already merged #32475 last week.

That said, I think we can close this issue out since it was a compile-time-only change and we didn't use a C+ so no additional payments are due

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

No branches or pull requests

3 participants