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

no-restricted-modules doesn't check es6 modules #3196

Closed
zaggino opened this Issue Jul 30, 2015 · 14 comments

Comments

Projects
None yet
8 participants
@zaggino
Copy link

zaggino commented Jul 30, 2015

setting
no-restricted-modules: [2, "cluster"]
doesn't throw error on
import cluster from "cluster";
probably due to:

if (isRequireCall(node)) {

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Jul 30, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@IanVS IanVS added the triage label Jul 30, 2015

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Jul 30, 2015

Yeah that sounds like an oversight to me.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 30, 2015

Not an oversight - this was created for require syntax. We generally do not treat require the same as ES6 modules because they behave differently.

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Jul 30, 2015

@nzakas because it's called no-restricted-modules, would it be ok to add import declarations to the list of places that could trigger the warning. I don't think it makes sense for people to make that list twice.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 30, 2015

Wouldn't people be using one or the other? Not both?

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Jul 30, 2015

Not in our case where we have a transitioning codebase, but I know you've
been consistent about this elsewhere so I'm fine with a new rule...

On Thu, Jul 30, 2015 at 2:13 PM Nicholas C. Zakas notifications@github.com
wrote:

Wouldn't people be using one or the other? Not both?


Reply to this email directly or view it on GitHub
#3196 (comment).

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Jul 31, 2015

I'd be really disappointed if this rule couldn't handle both styles.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 2, 2015

I'm just really worried about bundling these together when we don't know what format module strings will ultimately take. People "using" ES6 modules today are doing so built on the back of require, but there's no guarantee that module strings will be in the same format when native implementations arrive.

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Aug 3, 2015

If we do make a new rule, I guess we'd have to create a new rule and then deprecate this one?

  • no-require-modules
  • no-import-modules

One argument in favor of making them separate: AMD modules. If we wanted to add a no-define-modules rule it would be really easy to add that without having a fight as to if it's worth overloading the no-restricted-modules rule.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 4, 2015

No, I don't see any reason to deprecate the existing one, I'd just add no-restricted-imports

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Sep 9, 2015

I think the decision has been made and we need label updates?
CC @nzakas

@nzakas nzakas added rule accepted feature and removed triage labels Sep 9, 2015

guyellis added a commit to guyellis/eslint that referenced this issue Dec 1, 2015

@guyellis

This comment has been minimized.

Copy link
Contributor

guyellis commented Dec 1, 2015

I'm working on this.

Question. Trying to get debug to work while working on this. Added the expected require and calls in the code and ran with:

DEBUG=eslint:no-restricted-imports ./node_modules/.bin/mocha tests/lib/rules/no-restricted-imports.js

However can't produce any debug output. Any ideas on how to enable debug output when running a test?

Here is where I'm referencing it:
https://github.com/guyellis/eslint/blob/no-restricted-imports/lib/rules/no-restricted-imports.js#L7

And here is where I'm using it:
https://github.com/guyellis/eslint/blob/no-restricted-imports/lib/rules/no-restricted-imports.js#L37

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 1, 2015

Seems like that should work. Maybe try DEBUG=eslint:* instead? (Avoid typos)

guyellis added a commit to guyellis/eslint that referenced this issue Dec 2, 2015

guyellis added a commit to guyellis/eslint that referenced this issue Dec 2, 2015

@guyellis

This comment has been minimized.

Copy link
Contributor

guyellis commented Dec 2, 2015

PR submitted: #4590

This is my first PR for ESLint. Let me know if it needs finessing.

guyellis added a commit to guyellis/eslint that referenced this issue Dec 2, 2015

guyellis added a commit to guyellis/eslint that referenced this issue Dec 2, 2015

@nzakas nzakas closed this in 0ad7b93 Dec 4, 2015

nzakas added a commit that referenced this issue Dec 4, 2015

Merge pull request #4590 from guyellis/no-restricted-imports
New: Add no-restricted-imports rule (fixes #3196)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.