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

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

Open
Tracked by #2716
johnvonneumann7 opened this issue Feb 12, 2023 · 14 comments
Open
Tracked by #2716

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

johnvonneumann7 opened this issue Feb 12, 2023 · 14 comments

Comments

@johnvonneumann7
Copy link

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
Copy link
Member

ljharb commented Feb 12, 2023

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

@snewcomer
Copy link
Contributor

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
Copy link

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
Copy link

merrywhether commented Feb 20, 2023

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?

  • If no, then I think the path forward is 1 above as that rule will first and then this rule can clean up on a second pass.
  • if yes, then I think the path forward is 2 above (or something like it) and augmenting the rule to somehow cross-compare the import spaces.

@snewcomer
Copy link
Contributor

@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!

#2716

@snewcomer
Copy link
Contributor

@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
Copy link
Member

ljharb commented Feb 20, 2023

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
Copy link

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
Copy link
Member

ljharb commented Feb 20, 2023

@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
Copy link

I think this is similar to #2675

@kevinwolfcr
Copy link

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
Copy link

alecmev commented Jul 26, 2023

Fixed by #2835?

sjdemartini added a commit to sjdemartini/mui-tiptap that referenced this issue Aug 7, 2023
In particular, glad to have
import-js/eslint-plugin-import#2715 resolved.
It caught one unnecessary redundancy already, auto-fixed here!
sjdemartini added a commit to sjdemartini/mui-tiptap that referenced this issue Aug 7, 2023
In particular, glad to have
import-js/eslint-plugin-import#2715 resolved.
It caught one unnecessary redundancy already, auto-fixed here!
@dylang
Copy link

dylang commented Sep 27, 2023

related? #2792

@juunzzi

This comment was marked as spam.

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

No branches or pull requests

10 participants