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-duplicate-imports] ignore namespace imports (fixes #12758) #12935

Closed
wants to merge 3 commits into from

Conversation

JoaoDsv
Copy link

@JoaoDsv JoaoDsv commented Feb 18, 2020

Prerequisites checklist

  • I have read the contributing guidelines.
  • The team has reached consensus on the changes proposed in this pull request. If not, I understand that the evaluation process will begin with this pull request and won't be merged until the team has reached consensus.

What is the purpose of this pull request? (put an "X" next to an item)

Solve this issue.

What changes did you make? (Give an overview)

Fix a bug on [no-duplicate-imports] rule.
It shouldn't throw an error when the duplicated import is a namespace import such as import * as Namespace from 'module'

What did you do? Please include the actual source code causing the issue.
Fix a bug on [no-duplicate-imports] rule.

What did you expect to happen?
It shouldn't throw an error when the duplicated import is a namespace import such as import * as namespace from 'module'. Because namespace imports can't be merged.

Example of correct code:

import { anotherValue } from 'parsers';
import * as parsers1 from 'parsers';

What actually happened? Please include the actual, raw output from ESLint.
It was considered as incorrect.

Is there anything you'd like reviewers to focus on?

I'm wondering if it could be helpful to extract isNamespaceImport() function's content inside an util. WDYT?

Transforming it a bit to be more generic, it should look like this:

/**
 * Checks the specifier's type.
 * @param {ASTNode} node A node to check.
 * @param {string} the type of the import we want to check .
 *
 * @returns {boolean} Whether or not the specifier's type matchs the "type" param.
 */
function checkSpecifierType(node, type) {
    return (
        node.specifiers &&
        node.specifiers[0] &&
        node.specifiers[0].type === type 
    );
}

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Feb 18, 2020
@JoaoDsv JoaoDsv changed the title Issue 12758 Fix: [no-duplicate-imports] Treat namespace imports as different (fixes #12758) Feb 18, 2020
@JoaoDsv JoaoDsv changed the title Fix: [no-duplicate-imports] Treat namespace imports as different (fixes #12758) Fix: [no-duplicate-imports] ignore namespace imports (fixes #12758) Feb 18, 2020
/*eslint no-duplicate-imports: "error"*/

import * as Namespace from 'module';
import { merge } from 'module';
Copy link
Contributor

Choose a reason for hiding this comment

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

This behaviour doesn't fit the heuristic:

an import that can be merged with another is a duplicate of that other.

Namespace imports and named imports can be merged with default imports.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your feedback @G-Rath
I'll rework on it to fit this scenario

@nzakas nzakas linked an issue Feb 18, 2020 that may be closed by this pull request
@nzakas nzakas added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Feb 18, 2020
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Thanks for putting this pull request together. I think you may have the desired functionality reversed, but you're not too far off.

Don't worry about extracting isNamespaceImport() into a general utility. That's something that can always be done later if we need it.

@nzakas
Copy link
Member

nzakas commented Feb 28, 2020

@JoaoDsv are you still working on this?

@JoaoDsv
Copy link
Author

JoaoDsv commented Mar 5, 2020

@nzakas I had no time to work on it since your feedback, but I'd be happy to resume this PR next week if it sounds still relevant (and not too late) for you.

@nzakas
Copy link
Member

nzakas commented Mar 7, 2020

Not too late at all. Just wanted to be sure we give you a chance to finish up before someone else takes this on. If you’re not able to look at it next week, please let us know.

@nzakas
Copy link
Member

nzakas commented Apr 7, 2020

@JoaoDsv just checking if are you still working on this? Totally understand if not, just want to be sure.

@nzakas
Copy link
Member

nzakas commented Jun 11, 2020

It looks like this pull request has been abandoned, so closing.

Note: the issue this PR addresses is still accepted (#12758) so anyone who wants to work on implementing the change may do so and submit a separate PR.

@nzakas nzakas closed this Jun 11, 2020
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 9, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[no-duplicate-imports] Treat namespace imports as different
3 participants