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

[Fix] no-duplicates with type imports #2716

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Feb 12, 2023

@snewcomer snewcomer force-pushed the sn/fix-no-duplicates branch 2 times, most recently from ab7a2d8 to d2292bb Compare February 12, 2023 20:13
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

and so on, all the loops should be iteration methods whenever possible :-)

src/rules/no-duplicates.js Outdated Show resolved Hide resolved
src/rules/no-duplicates.js Outdated Show resolved Hide resolved
src/rules/no-duplicates.js Outdated Show resolved Hide resolved
src/rules/no-duplicates.js Outdated Show resolved Hide resolved
src/rules/no-duplicates.js Outdated Show resolved Hide resolved
@simmo
Copy link
Contributor

simmo commented Feb 14, 2023

Am I correct this would fix #2675 ? If so this is my new favourite PR. 😄

If you need any testing done, I can probably help.

@runspired
Copy link

all the loops should be iteration methods whenever possible

Is there a reason for this guidance?

@ljharb
Copy link
Member

ljharb commented Feb 14, 2023

@runspired loops make code harder to understand and reason about. It's explained in the airbnb styleguide as well.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Look forward to the other 2 fixes :-)

src/rules/no-duplicates.js Outdated Show resolved Hide resolved
src/rules/no-duplicates.js Outdated Show resolved Hide resolved
@mrm007
Copy link

mrm007 commented Apr 8, 2023

Look forward to the other 2 fixes :-)

@ljharb @snewcomer I am very interested in these fixes, so I wanted to take a stab at the remaining TODO items. As it turns out, they were already covered by this PR!

I reorganised some tests (inspired by tests/src/rules/consistent-type-specifier-style.js) to prove that both TypeScript and Flow work — see snewcomer#1

Test results
no-duplicates

<snip>

with types
  Babel/Flow
    no-duplicates
      valid
        ✓ import type { x } from './foo'; import y from './foo'
        ✓ import type x from './foo'; import type y from './bar'
        ✓ import type {x} from './foo'; import type {y} from './bar'
        ✓ import type x from './foo'; import type {y} from './foo'
        ✓ 
      import type {} from './module';
      import {} from './module2';
    
        ✓ 
      import type { Identifier } from 'module';

      declare module 'module2' {
        import type { Identifier } from 'module';
      }

      declare module 'module3' {
        import type { Identifier } from 'module';
      }
    
        ✓ import { type x } from './foo'; import type y from 'foo'
      invalid
        ✓ import { type x } from './foo'; import y from './foo';
        ✓ import type x from './foo'; import type y from './foo'
        ✓ import type x from './foo'; import type x from './foo'
        ✓ import type {x} from './foo'; import type {y} from './foo'
        ✓ import {type x} from './foo'; import {y} from './foo'
        ✓ import type {x} from './foo'; import {type y} from './foo'
        ✓ import type {x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo'; import type {y} from './foo';
        ✓ import {type x} from 'foo'; import type {y} from 'foo'
        ✓ import {type x, C} from './foo'; import type {y} from './foo';
        ✓ import type {y} from './foo'; import {type x, C} from './foo';
        ✓ import {type x, C} from 'foo';import type {y} from 'foo';
        ✓ import type {y} from './foo'; import {type x, C} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import {type z} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import A from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import A, { C } from './foo';
        ✓ import {AValue, type x, BValue} from './foo';import {type y, CValue} from './foo';
        ✓ import AValue from './foo'; import {type y} from './foo';
        ✓ import {type y} from './foo';import AValue from './foo';
        ✓ import AValue, {BValue} from './foo'; import {type y, CValue} from './foo';
        ✓ import {AValue, type x, BValue} from './foo'; import type {y} from './foo';
        ✓ import {AValue, type x, BValue} from './foo'; import type {y} from './foo'
        ✓ import AValue, {type x, BValue} from './foo';import type {y} from './foo';
        ✓ import AValue, {type x, BValue} from './foo'; import type {y} from './foo'
        ✓ import { type C, } from './foo';import {AValue, BValue, } from './foo';
  TypeScript
    no-duplicates
      valid
        ✓ import type { x } from './foo'; import y from './foo'
        ✓ import type x from './foo'; import type y from './bar'
        ✓ import type {x} from './foo'; import type {y} from './bar'
        ✓ import type x from './foo'; import type {y} from './foo'
        ✓ 
      import type {} from './module';
      import {} from './module2';
    
        ✓ 
      import type { Identifier } from 'module';

      declare module 'module2' {
        import type { Identifier } from 'module';
      }

      declare module 'module3' {
        import type { Identifier } from 'module';
      }
    
        ✓ import { type x } from './foo'; import type y from 'foo'
      invalid
        ✓ import { type x } from './foo'; import y from './foo';
        ✓ import type x from './foo'; import type y from './foo'
        ✓ import type x from './foo'; import type x from './foo'
        ✓ import type {x} from './foo'; import type {y} from './foo'
        ✓ import {type x} from './foo'; import {y} from './foo'
        ✓ import type {x} from './foo'; import {type y} from './foo'
        ✓ import type {x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo'; import type {y} from './foo';
        ✓ import {type x} from 'foo'; import type {y} from 'foo'
        ✓ import {type x, C} from './foo'; import type {y} from './foo';
        ✓ import type {y} from './foo'; import {type x, C} from './foo';
        ✓ import {type x, C} from 'foo';import type {y} from 'foo';
        ✓ import type {y} from './foo'; import {type x, C} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import {type z} from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import A from './foo';
        ✓ import {type x} from './foo';import {type y} from './foo';import A, { C } from './foo';
        ✓ import {AValue, type x, BValue} from './foo';import {type y, CValue} from './foo';
        ✓ import AValue from './foo'; import {type y} from './foo';
        ✓ import {type y} from './foo';import AValue from './foo';
        ✓ import AValue, {BValue} from './foo'; import {type y, CValue} from './foo';
        ✓ import {AValue, type x, BValue} from './foo'; import type {y} from './foo';
        ✓ import {AValue, type x, BValue} from './foo'; import type {y} from './foo'
        ✓ import AValue, {type x, BValue} from './foo';import type {y} from './foo';
        ✓ import AValue, {type x, BValue} from './foo'; import type {y} from './foo'
        ✓ import { type C, } from './foo';import {AValue, BValue, } from './foo';


123 passing (560ms)

@niklasnatter
Copy link

Hey, it would be great to handle type imports with no-duplicates - Is there anything that I could help with?

@mrm007
Copy link

mrm007 commented May 23, 2023

Is there anything else we can help with to get this merged? It's a great improvement over the current behaviour.

@seanCodes
Copy link

Also eager to see this merged and happy to help where needed!

@oceandrama
Copy link

oceandrama commented Jul 14, 2023

@snewcomer Hi! Sorry for mention, but this PR is super helpfull and it will be great to see this merged! Are there any additional things you would like to work on?
Maybe it also fixes #2792

@niklasnatter
Copy link

I havent tested yet, but this might be fixed by #2835

@ljharb
Copy link
Member

ljharb commented Jul 27, 2023

I've rebased this PR, but tests seem to be failing. @mrm007 sorry i hadn't looked at your PR-PR yet - would you mind rebasing yours, and then I can pull it in?

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

@snewcomer is there a reason you overwrote my rebase?

@snewcomer
Copy link
Contributor Author

Hi sorry. I didn't see any commits. If I was mistaken due to keeping this PR open in Safari and not getting real time updates, I'm sorry.

FYI put a comment here.

f302f7d#r123760801

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

Successfully merging this pull request may close these issues.

8 participants