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] Properly autofix first default and rest named imports. #1431

Open
wants to merge 3 commits into
base: master
from

Conversation

@umidbekkarimov
Copy link

commented Jul 31, 2019

Closes #1403

@coveralls

This comment has been minimized.

Copy link

commented Jul 31, 2019

Coverage Status

Coverage decreased (-0.06%) to 97.841% when pulling 639f441 on umidbekkarimov:fix-1403 into 989f6cc on benmosher:master.

@umidbekkarimov

This comment was marked as resolved.

Copy link
Author

commented Aug 1, 2019

Not sure what caused a timeout 🤔First commit passed, and the second one had only some refactorings.


// #1403: first default and rest named imports should be merged
test({
code: "import def, {x} from './foo'; import {y} from './foo';",

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 1, 2019

Collaborator

can we add some tests that include merging multiple default imports of the same name, and also like 3 or 4 different import statements bringing in names?

This comment has been minimized.

Copy link
@umidbekkarimov

umidbekkarimov Aug 1, 2019

Author

can we add some tests that include merging multiple default imports of the same name,

Multiple default imports from the same module will be bailed by https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-duplicates.js#L53

and also like 3 or 4 different import statements bringing in names?

I didn't quite understand what you mean by that. I checked with

import { x, y } from './foo'; import { y, z } from './foo';

and was it fixed to

import { x, y , y, z } from './foo' 

And I think that it's right because developer should handle duplicated variables.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 1, 2019

Collaborator

Why? If they’re the same value and the same name and they’re already being merged, they should be uniquified.

Also, what about named imports that rename using as?

This comment has been minimized.

Copy link
@umidbekkarimov

umidbekkarimov Aug 1, 2019

Author

Oh, I see your point now.

// before
import def from './foo'; import {default as x} from './foo';
// after
import def, {default as x} from './foo'; 

// before
import { x } from './foo'; import { x as y } from './foo';
// after
import { x , x as y } from './foo'; 

I just thought that this not fits in the scope of the current pull request.

@@ -86,7 +86,8 @@ function getFix(first, rest, sourceCode) {
!specifiers.some(specifier => specifier.importNode === node)
)

const shouldAddDefault = getDefaultImportName(first) == null && defaultImportNames.size === 1
const firstDefaultSpecifier = getDefaultImportSpecifier(first)

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 1, 2019

Collaborator

what about import x from 'path'; import y from 'path';? We can't just safely merge them into x.

This comment has been minimized.

This comment has been minimized.

Copy link
@ljharb

ljharb Aug 1, 2019

Collaborator

Sure but i explicitly want tests for multiple defaults and separate nameds, so we can see what that does.

Many tests test multiple behaviors; the important thing is that we have as many tests as possible :-)

This comment has been minimized.

Copy link
@umidbekkarimov

umidbekkarimov Aug 1, 2019

Author

Ok, I'll try to find some unimplemented edge cases, but I think I will have to refactor some things (if not everything) in order to fix them

@umidbekkarimov

This comment has been minimized.

Copy link
Author

commented Aug 2, 2019

Here another attempt. But it comes with a question - how to handle comments in specifiers?

The previous solution was copying content from {...} and concatenating it to others. To have control over duplicated specifiers, I had to create a Set of all specifiers and create a new import node from scratch.

import { /* a */ a /* a */ } from 'foo';
import { /* b */ a /* b */ } from 'foo';

// this? (unify by specifier text)
import { /* a */ a /* a */ } from 'foo';

// or that? (unify by specifier text with comments)
import { /* a */ a /* a */,  /* b */ a /* b */ } from 'foo';


// And what to do with `eslint-ignore`
import {  
a,
// eslint-ignore some-other rule
b
 } from 'foo';

I have little experience with eslint rules, and not sure about correct way to hande comments, so I just ignored all comments for now.

@ljharb

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2019

The safest is probably to not autofix any lines adjacent to comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.