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

[consistent-type-imports] prefer inline type import syntax #4338

Closed
akphi opened this issue Dec 21, 2021 · 17 comments · Fixed by #5050
Closed

[consistent-type-imports] prefer inline type import syntax #4338

akphi opened this issue Dec 21, 2021 · 17 comments · Fixed by #5050
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@akphi
Copy link

akphi commented Dec 21, 2021

Description

I would like to propose an extension to the existing consistent-type-imports rule. This is linked to the work done in #4237

I think it would be beneficial for people who are interested in adopting the new inline type import syntax introduced in Typescript 4.5. We can introduce a flag, for example preferInlineTypeImports?: boolean.

Like @bradzacher mentioned, it's tricky to do a fixer, so we don't need it for now, but at least a way to warn people about violation would be great.

Fail

import type { c } from './c';
import { a } from './a'; 
import type { b } from './a';

Pass

import type { c } from './c';
import { a, type b } from './a';

Let me know what you think about this. If you give me a green light, I think if I can try working on this feature.

@akphi akphi added enhancement: new base rule extension New base rule extension required to handle a TS specific case package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Dec 21, 2021
@akphi akphi changed the title Rule proposal: [consistent-type-imports] prefer mixed type import syntax Rule proposal: [consistent-type-imports] prefer inline type import syntax Dec 21, 2021
@bradzacher
Copy link
Member

bradzacher commented Dec 21, 2021

As I mentioned in my comment here: #3950 (comment)
I don't think this rule should be in the business of condensing imports when they are correctly "typed".
Instead that would be the concern of a rule like no-duplicate-imports (or even better would be import/no-duplicates - #4238 - because ESLint's base rule doesn't support typed imports as it's not "pure JS" syntax).

EG this code:

import type { cType } from './c';
import { aValue } from './a'; 
import type { bType } from './a';

Or this code:

import type { cType } from './c';
import { aValue, type bType } from './a'; 

Would never error in this rule - regardless of config - because all the types are imported via type imports.


However, I do think that this lint rule's fixer could very much be concerned with fixing to "condensed" imports when configured to do so.
I'd accept option like fixStyle which works like this

  • separate-type-imports
    • always fix type imports to standalone import type { ... } statements.
    • reuse existing import type { ... } if one exists for the module.
    • This is the current behaviour, and would be the default for the option (or else it'd be a breaking change).
  • inline-type-imports
    • reuse existing import type { ... } if one exists for the module, if one exists.
    • otherwise - always fix type imports to inline import { type x, ... }.
  • mixed - combine the above:
    • reuse existing import type { ... } if one exists for the module, if one exists.
    • if there are no value imports for a module, prefer import type { ... }.
    • if there are value imports for a module, prefer import { type x, ... }.

(option names are up to whomever implements - the above are just an example)

Fixed output example:

import type { dType1 } from './d';
import { dType2 } from './d';
import { cType } from './c';
import { aValue, bType } from './a'; 

// `separate-type-imports` fix to
import type { dType1, dType2 } from './d';
import type { cType } from './c';
import type { bType } from './a';
import { aValue } from './a'; 

// `inline-type-imports` fix to
import type { dType1, dType2 } from './d';
import { type cType } from './c';
import { aValue, type bType } from './a'; 

// `mixed` fix to
import type { dType1, dType2 } from './d';
import type { cType } from './c';
import { aValue, type bType } from './a'; 

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for maintainers to take a look enhancement: new base rule extension New base rule extension required to handle a TS specific case labels Dec 21, 2021
@akphi
Copy link
Author

akphi commented Dec 21, 2021

Yes, totally agree with having this in no-duplicate-imports instead. Do you accept a PR to add a config to that rule? Something like ignoreTypeImports?: boolean. I'm honestly not sure of the naming here.

Regarding the fixer, like you said, it could be a little complicated, so I might not pursue it here. But I want to bring to your attention, the inline-type-imports fix might not work well. Please see the discussion here - microsoft/TypeScript#47118

Basically

import { type cType } from './c';

will cause a Typescript compilation error when importsNotUsedAsValues: true if there is no non-type imports.

@bradzacher bradzacher changed the title Rule proposal: [consistent-type-imports] prefer inline type import syntax [consistent-type-imports] prefer inline type import syntax Dec 21, 2021
@bradzacher
Copy link
Member

Yes, totally agree with having this in no-duplicate-imports instead. Do you accept a PR to add a config to that rule?

I don't believe we should be maintaining our extension rule, no-duplicate-imports - #4238.
There's no reason for us to do so when there is a much better version of the rule in the ecosystem which already supports things - import/no-duplicates.

I would like to remove our extension rule in the next major, so I don't hugely want us to add more functionality into it for now.

@akphi
Copy link
Author

akphi commented Dec 21, 2021

@bradzacher that's a very fair point! I totally forgot about this base rule. I will close this issue now.

@akphi akphi closed this as completed Dec 21, 2021
@bradzacher bradzacher reopened this Dec 21, 2021
@bradzacher
Copy link
Member

I'll leave this open as a tracking issue in case someone wants to implement the changes to the fixer.

@gajus
Copy link

gajus commented Apr 6, 2022

You can now achieve this with canonical/prefer-inline-type-import.

@snewcomer
Copy link
Contributor

I believe this is still something we want in this repo even with the additions to the canonical? If so, I've started working on a fix.

@snewcomer

This comment was marked as off-topic.

@Josh-Cena

This comment was marked as off-topic.

@snewcomer

This comment was marked as off-topic.

@Josh-Cena

This comment was marked as off-topic.

@snewcomer

This comment was marked as off-topic.

@Josh-Cena

This comment was marked as off-topic.

@blingerson
Copy link

Is there any way to disable the fixer for consistent-type-imports in the meantime? The fixer just ends up causing hundreds of new issues when used in conjunction with import/no-duplicates.

@snewcomer
Copy link
Contributor

@blingerson Could you detail some of the issues you found and perhaps we can fix them? Even better in a new issue!

@bradzacher
Copy link
Member

import-js/eslint-plugin-import#2469 has been merged but not yet released.

@snewcomer
Copy link
Contributor

@bradzacher Amazing!

Just to note, we have 2/3 left to get proper inline imports. This PR is next I believe.

import-js/eslint-plugin-import#2475

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
6 participants