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

change: [M3-6541] - eslint: consistent-type-imports #10540

Merged
merged 3 commits into from
Jun 5, 2024

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Jun 3, 2024

Description 📝

Tiny PR to start enforcing consistent-type-imports through the codebase (import type for type file)

https://typescript-eslint.io/rules/consistent-type-imports

This will remain a warning it does not affect the developer experience negatively other than suggesting to make that change when creating or editing a file. It will also make it easier/quicker to fix those via shortcuts when encountered.

Changes 🔄

  • Add consistent-type-imports warning to our es-lint rules

Preview 📷

Include a screenshot or screen recording of the change

Screenshot 2024-06-03 at 12 39 07

How to test 🧪

Verification steps

  • Pull locally and open random file to see the new lint warning

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai self-assigned this Jun 3, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review June 3, 2024 16:51
@abailly-akamai abailly-akamai requested a review from a team as a code owner June 3, 2024 16:51
@abailly-akamai abailly-akamai requested review from hana-linode and carrillo-erik and removed request for a team June 3, 2024 16:51
Copy link

github-actions bot commented Jun 3, 2024

Coverage Report:
Base Coverage: 82.36%
Current Coverage: 82.36%

@bnussman-akamai
Copy link
Contributor

Curious to hear your thoughts on microsoft/TypeScript#39861 (comment)

@abailly-akamai
Copy link
Contributor Author

@bnussman-akamai I agree it's mostly stylistic at this point (i was convinced there was another element to it, but maybe not or irrelevant to our bundling/compiling setup).

I think that comment is obsolete however, since

and since there are currently no tools that would enforce this style, it would fall on you to analyze and separate your declarations manually, wasting your valuable coding time.

isn't true anymore.

Thing is, we have agreed and started to implement this style (and people usually point to it in code reviews), so this PR is for consistency sake. Not a hill i would die on, I was again mostly going for consistency here. I don't even know if we can go back to grouping imports via linting, so our options are: we leave things as is and we don't think about it and stop commenting on PRs OR we go with the lint rule.

Copy link
Contributor

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

New rules works as expected ✅

Copy link
Contributor

@mjac0bs mjac0bs 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 improving the developer experience - confirmed the linter is flagging these instances as a warning.

Screenshot 2024-06-05 at 10 49 20 AM

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Jun 5, 2024
@abailly-akamai abailly-akamai merged commit d52ad40 into linode:develop Jun 5, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants