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

Enforce type in front of type-only imports #2618

Open
1 of 10 tasks
pokey opened this issue Aug 1, 2024 · 1 comment
Open
1 of 10 tasks

Enforce type in front of type-only imports #2618

pokey opened this issue Aug 1, 2024 · 1 comment
Labels
code quality Improvements to code quality

Comments

@pokey
Copy link
Member

pokey commented Aug 1, 2024

We want to proceed in the following steps, each in its own PR, in the following exact order:

  • enable "@typescript-eslint/consistent-type-imports": "error"
  • enable --verbatimModuleSyntax TS flag
  • enable "import/consistent-type-specifier-style": ["error", "prefer-top-level"]
  • enable "import/no-duplicates": "error" (note we don't need prefer-inline because we don't use inline)

The reasoning is that we want import type to be like its own kind of statement that we know always gets dropped in the output Javascript, and in addition, if we were to have a rule where import {type foo} => import type {foo} only when there are no non-type imports in the statement, it would feel a bit inconsistent that it doesn't happen when there are non-type imports. The proposed rule is simpler: types always get imported in their own statement, and never have side effects

Each step should be its own PR, and they should be in the above order. Note that 1) must come before 2) because without 2), 1) should have no side effects, and then the effects of 2) should become more clear once 1) is in

Note that we don't need no-import-type-side-effects lint rule because 2 and 3 won't allow us to have inline type anywhere

Before merging each PR, we want to test the following cases:

  • you use something as a type but not runtime and it's currently imported as runtime
  • you use something as a type that is not yet imported
  • you use something as runtime that is currently imported as a type

For each of the above cases, we want to test:

  • Auto-fix
  • Organize imports
  • Auto-import

Originally posted by @pokey in #1832 (comment)

@pokey
Copy link
Member Author

pokey commented Aug 1, 2024

Originally posted by @auscompgeek in https://github.com/cursorless-dev/cursorless/pull/1832/files#r1399814887:

I still never got a chance to test that matrix, but up until TypeScript 5.3 whether auto-import used import type depends on the tsconfig. TypeScript 5.3 adds an IDE setting to prefer auto-importing with import type https://devblogs.microsoft.com/typescript/announcing-typescript-5-3/#settings-to-prefer-type-auto-imports

The presence of this lint rule will obviously not affect auto-import behaviour since eslint doesn't change the tsconfig. The auto-import behaviour probably isn't too big of an issue anyway since we all have eslint autofix on save, I believe.

github-merge-queue bot pushed a commit that referenced this issue Aug 2, 2024
related #2618
revives #1832

vscode quick fix import and auto import appears to be doing the right
thing in regards to type only or runtime imports. Imports organize or
imports fix unfortunately do not convert a runtime import to a type
import if it's only used as a type. We probably need to rely on the
linter to do that part for us.

## Checklist

- [/] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [/] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [/] I have not broken the cheatsheet

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
@auscompgeek auscompgeek added the code quality Improvements to code quality label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality Improvements to code quality
Projects
None yet
Development

No branches or pull requests

3 participants