[import/no-duplicates] Properly autofix first default and rest named imports. #1431
Conversation
This comment has been minimized.
This comment has been minimized.
|
|
||
| // #1403: first default and rest named imports should be merged | ||
| test({ | ||
| code: "import def, {x} from './foo'; import {y} from './foo';", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/rules/no-duplicates.js
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
what about import x from 'path'; import y from 'path';? We can't just safely merge them into x.
There was a problem hiding this comment.
I think it's already covered in https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/rules/no-duplicates.js#L55-L57 by https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-duplicates.js#L53.
I may seem confusing because a single test checks two behaviors.
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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
|
Here another attempt. But it comes with a question - how to handle comments in specifiers? The previous solution was copying content from 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. |
|
The safest is probably to not autofix any lines adjacent to comments. |
Closes #1403