import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.58k stars 1.57k forks source link

[import/no-duplicates] `{"prefer-inline": true}` does not work #2715

Open johnvonneumann7 opened 1 year ago

johnvonneumann7 commented 1 year ago

version 2.27.5

https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-duplicates.md#inline-type-imports The documentation states that adding {"prefer-inline": true} as shown below will cause errors and autofixes if there are duplicate imports. However, this was not possible in my environment.

{
  "import/no-duplicates": ["error", {"prefer-inline": true}],
}

It should look like the following, but there is no error or anything.

import NextErrorComponent from 'next/error';
import { type ErrorProps } from 'next/error';
// ↓
import NextErrorComponent, { type ErrorProps } from 'next/error';

image

Is this currently being corrected?

ljharb commented 1 year ago

I agree this should be autofixed - as well as warned on. cc @snewcomer

snewcomer commented 1 year ago

Ok perfect. Been working on and throwing away solutions for a few weeks now here and there. Will try to put up my progress today.

guillaumebrunerie commented 1 year ago

Related issue here, when running ESLint from the terminal I get the error

Configuration for rule "import/no-duplicates" is invalid:
Value {"prefer-inline":true} should NOT have additional properties.

even though I used the exact same options as in the docs.

merrywhether commented 1 year ago

The issue lies in the creation of the various import maps in the rule. In the case of the following:

import { type Foo } from '.';
import { foo } from '.';

Line 1 will be sorted into namedTypesImported and Line 2 will be imported into imported, and then when the checkImports function is run later neither of each of the maps' nodes will have length > 1. Currently the rule's example only works because both lines have type imports in them.

If I wrap this block in a if (!prefersInline) check, the above correctly collapses into

import { foo , type Foo } from '.';

I need to dig more though as I'm not yet sure which is the correct description of the problem:

  1. It's not correct to ever sort the inline imports into the *TypesImported maps because those are for managing import type ... type-style import statements which can't be directly compared with import ... value-style import statements. In this case, the wrapped block above is not necessary at all?
  2. Imports with inline type imports should get added to multiple maps. Then we compare types against all types and values against all values, but I'm not sure what happens in a multi-way tie (I haven't grokked the full rule enough yet).

It's likely that the full spectrum of cases needs to be first enumerated. Like what happens in the following situation with and without prefers-inline:

import type { Foo } from '.';
import { foo, type bar } from '.';
import { bar } from '.';

Given the existence of import/consistent-type-specifier-style, should this rule ever concern itself with comparing lines 1 and 2?

snewcomer commented 1 year ago

@merrywhether Could you try and symlink this PR? I still have to run it against a few repos to see if it works but it would be good to get some early signal!

https://github.com/import-js/eslint-plugin-import/pull/2716

snewcomer commented 1 year ago

@guillaumebrunerie

Related issue here, when running ESLint from the terminal I get the error

Have you updated the eslint-plugin-import to a version that it is supported. We had an issue where the docs were published before the feature was released. In any case, I would avoid upgrading anyways. I made a mistake with the original PR where I still had the typescript-eslint repo linked where I did the bundled work so got a false positive that the amalgamation of PRs worked :(. no-duplicates It is pretty broken right now.

ljharb commented 1 year ago

We had an issue where the docs were published before the feature was released.

That's not entirely accurate; docs are always landed on main prior to being released - users need to check the tagged docs (on every repo on github, not just this one)

guillaumebrunerie commented 1 year ago

Oh, it can very well be that I did not upgrade, I didn't know it was a new feature. But in that case the error message is very confusing, I think it should say something like Unknown rule "import/no-duplicates". or similar.

ljharb commented 1 year ago

@guillaumebrunerie you'd need to ask eslint about that; we have no control over that error message. In this case, the rule was known, but that schema option was not.

niklasnatter commented 1 year ago

I think this is similar to https://github.com/import-js/eslint-plugin-import/issues/2675

kevinwolfcr commented 1 year ago

Having this issue too. It only detects an error when an existing import already has types, like this one:

import { AValue, type AType } from './mama-mia'
import type { BType } from './mama-mia'

Gets fixed into this:

import { type AType, AValue, type BType } from "./mama-mia"

However, if first import doesn't have a type import already, like this:

import { AValue } from './mama-mia'
import type { BType } from './mama-mia'

It doesn't work on that case.

alecmev commented 1 year ago

Fixed by https://github.com/import-js/eslint-plugin-import/pull/2835?

dylang commented 1 year ago

related? https://github.com/import-js/eslint-plugin-import/issues/2792

juunzzi commented 4 months ago

help me please..