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

Rule proposal: no-wildcard-import #4865

Closed
silvenon opened this issue Jan 6, 2016 · 28 comments
Closed

Rule proposal: no-wildcard-import #4865

silvenon opened this issue Jan 6, 2016 · 28 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Comments

@silvenon
Copy link
Contributor

silvenon commented Jan 6, 2016

Found this rule in Airbnb's JS styleguide and I liked it.

@eslintbot
Copy link

@silvenon Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Jan 6, 2016
@silvenon
Copy link
Contributor Author

silvenon commented Jan 6, 2016

  1. When does this rule warn? Please describe and show example code:

    It warns when you use a wildcard import.

    import * as foo from 'foo';
  2. Is this rule preventing an error or is it stylistic?

    Stylistic.

  3. Why is this rule a candidate for inclusion instead of creating a custom rule?

    Not sure what this question means 😕

  4. Are you willing to create the rule yourself?

    Yes.

@nzakas
Copy link
Member

nzakas commented Jan 7, 2016

Why is this rule a candidate for inclusion instead of creating a custom rule?

In other words, why should this be part of the ESLint core and managed by us when you can write and manage it yourself?

@nzakas nzakas added rule Relates to ESLint's core rules feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jan 7, 2016
@silvenon
Copy link
Contributor Author

silvenon commented Jan 7, 2016

After giving it some more thought, I don't think it's a good rule to enforce. Forcing a single default export instead of named exports provides no benefit.

@silvenon silvenon closed this as completed Jan 7, 2016
@michaelficarra
Copy link
Member

That syntax doesn't import the default export, it imports all exports.

@silvenon
Copy link
Contributor Author

silvenon commented Jan 7, 2016

Yes. I meant that not allowing wildcard imports will force you to write modules with a single (default) export.

@michaelficarra
Copy link
Member

No it won't.

@silvenon
Copy link
Contributor Author

silvenon commented Jan 7, 2016

// foo.js
export const a = 5;
export const b = 6;
// bar.js
import * as foo from 'foo'; // oops, error, fixing...
import foo from 'foo'; // but that won't work, gotta rewrite foo.js...
// foo.js
export default {
  a: 5,
  b: 6
};
// bar.js
import foo from 'foo'; // this works now
console.log(foo.a);

@silvenon
Copy link
Contributor Author

silvenon commented Jan 7, 2016

Oh, I see what you meant, when you're importing only certain exports, it doesn't really matter whether it's a single exported object or a named export.

@michaelficarra
Copy link
Member

Yes.

import {a, b} from "foo";

@jackson-sandland
Copy link

What about the case when you are importing actions into a component? e.g. import * as awesomeActions from './actions/awesomeActions';

@csvan
Copy link

csvan commented Dec 28, 2017

After giving it some more thought, I don't think it's a good rule to enforce.

I disagree. Stylistic benefits aside, disallowing wildcard imports has several technical implications. For example, wildcard imports breaks treeshaking in Webpack (I know not everyone uses Webpack, and that this may not be reason enough to have it as a core rule, but still).

@ljharb
Copy link
Sponsor Contributor

ljharb commented Dec 28, 2017

Can this be reopened? I'd very much like to ban wildcard imports in all of my projects.

@silvenon silvenon reopened this Dec 28, 2017
@platinumazure
Copy link
Member

Can this be enforced with no-restricted-syntax?

@j-f1
Copy link
Contributor

j-f1 commented Dec 28, 2017

Can this be enforced with no-restricted-syntax?

Yes — just restrict ImportNamespaceSpecifier.

@silvenon
Copy link
Contributor Author

silvenon commented Dec 28, 2017

I hadn't thought about that, good idea!

{
  "no-restricted-syntax": [
    "error",
    {
      "selector": ":matches(ImportNamespaceSpecifier, ExportAllDeclaration, ExportNamespaceSpecifier)",
      "message": "Import/export only modules you need"
    }
  ]
}

I haven't tested this, but it should prohibit all wildcard import and export statements:

import * as foo from 'foo';
export * from 'foo';
export * as foo from 'foo';

Let me know if that works for you, @ljharb.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Dec 28, 2017

It can, but that’s a blunt instrument that works very poorly with shared configs - since anyone wishing to modify any of the config for that rule needs to copy-paste the entire config and then tweak, to do it properly.

I think import * syntax is special enough, and will warrant special options like “don’t warn if you’re reflecting on the ModuleRecord object”, that it needs its own rule.

@silvenon
Copy link
Contributor Author

silvenon commented Dec 29, 2017

Also, people will still often want to do import * as React from 'react' since that is recommended in Flow docs. Yeah, that's a huge tradeoff… I think no-restricted-syntax should be used for creating project/organization-specific rules which usually don't make sense elsewhere. Wildcard import is enough of an antipattern to warrant its own rule.

How would configuration for this new rule look like? What would whitelisting look like?

@platinumazure what do you think?

@not-an-aardvark
Copy link
Member

It can, but that’s a blunt instrument that works very poorly with shared configs - since anyone wishing to modify any of the config for that rule needs to copy-paste the entire config and then tweak, to do it properly.

I agree. I think it would be a good idea to improve this situation (e.g. with #9192) so that no-restricted-syntax can be improved for all cases.

I think import * syntax is special enough, and will warrant special options like “don’t warn if you’re reflecting on the ModuleRecord object”, that it needs its own rule.

I disagree, this rule seems completely redundant with no-restricted-syntax to me. If no-restricted-syntax is inconvenient to use, then I think we should work on fixing that directly rather than adding a bunch of redundant rules in the meantime. The point of no-restricted-syntax is to make rules like this unnecessary.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Dec 29, 2017

I don’t think it makes them unnecessary; however, it should be able to make their implementation simply defer to that of no-restricted-syntax. There’s not actually much value in minimizing the number of distinct rules; whereas there is much value in abstracting away complex configuration (especially anything that deals with AST nodes) from eslint users.

@ilyavolodin
Copy link
Member

ilyavolodin commented Dec 29, 2017

There’s not actually much value in minimizing the number of distinct rules; whereas there is much value in abstracting away complex configuration

Completely disagree. ESLint has a huge number of rules. It's incredibly hard to find a rule that enforces specific pattern. Even as one of the maintainer, I have to constantly dig through documentation whenever creating my own config from the scratch. There is a very substantial value in minimizing the number of distinct rules from my perspective.

@Chaoste
Copy link

Chaoste commented May 28, 2018

I wouldn't completely disagree. Imagine you would have only 10 eslint rules which are capable of replacing any other rule - I can't imagine that everyone is able to understand the logic/language you would need to implement for this. I see no reason why making things complicated and screw up this nice module.

If a programmer installs eslint and looks for this feature we won't say "Hey great. I have to learn so much stuff before I can start !". Vice versa imagine he would google for this rule and find it directly. In my opinion a lint library shouldn't be minimalistic. I agree that it should provide advanced rules for advanced users with very special rules. But it should also abstract away complexity by providing simple rules.

Btw: if you're always starting your config from scratch you're definitely doing something wrong...

@Chaoste
Copy link

Chaoste commented May 28, 2018

For those who are wondering how to add this rule and add an exception for Reacts wildcard import:

no-restricted-syntax:
    - 2
    - selector: :matches(ImportNamespaceSpecifier[local.name!='React']) # just add another square bracket for a second name matching
      message: Import/export only modules you need
{
  "no-restricted-syntax": [
    "error",
    {
      "selector": ":matches(ImportNamespaceSpecifier[local.name!='React'])",
      "message": "Import/export only modules you need"
    }
  ]
}

Took me some time to make this work since the documentation on selectors is missing a lot of configurations (like the parameters of ImportNamespaceSpecifier). If you want to add similar rules or would like to see what options are available beside the documentation:

  1. List of available selectors [Code]

  2. Some code snippets from babel-generator making clear which parameters are available.

Greetings,
Thomas

@ljharb
Copy link
Sponsor Contributor

ljharb commented May 28, 2018

(Not sure why you’d need an exception for React; import React from ‘react’ is the proper way to import it)

@michaelficarra
Copy link
Member

@Chaoste See https://github.com/estree/estree for the tree documentation.

@Chaoste
Copy link

Chaoste commented May 28, 2018

Thanks for the feedback! If it's not necessary I would also remove the exception.

And thanks for pointing out the background of AST selectors - didn't know what this is all about ;)

@michaelficarra
Copy link
Member

@Chaoste Also see https://github.com/estools/esquery for the selector syntax.

@not-an-aardvark
Copy link
Member

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is a strong consensus among the team that they're worth adding.

Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement the rule yourself, rather than using a bundled rule that is packaged with ESLint.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 19, 2019
@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 Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests