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-unresolved not working for require #90

Closed
mctep opened this issue Nov 6, 2015 · 11 comments · Fixed by #100
Closed

no-unresolved not working for require #90

mctep opened this issue Nov 6, 2015 · 11 comments · Fixed by #100
Milestone

Comments

@mctep
Copy link

mctep commented Nov 6, 2015

Hello!
Thank you for your job.

My file:

require('./foo.js');
import './foo.js';

When executing

eslint --no-eslintrc --parser babel-eslint --plugin import --rule '{"import/no-unresolved":2}' test.js

I get only one error for import declaration:

~/test.js
  2:8  error  Unable to resolve path to module './foo.js'  import/no-unresolved

✖ 1 problem (1 error, 0 warnings)
@benmosher
Copy link
Member

I'm not sure what you're expecting. The no-unresolved rule only lints import statement paths; it will not inspect require calls.

@benmosher
Copy link
Member

Marking this invalid and closing for now, but if I've misunderstood, let me know.

@benmosher
Copy link
Member

Actually, upon reflection, this seems like a good improvement on the rule, and it shouldn't be tough to add. 👍

@benmosher benmosher reopened this Nov 8, 2015
@benmosher benmosher added this to the 0.10.0 milestone Nov 9, 2015
@benmosher
Copy link
Member

Agh... on second thought, require is a bit of a different animal, since it's a dynamic CallExpression... I think in the interest of simplicity, I'm going to re-mark this invalid and close. (at least for now... I may start doing more around CommonJS soon.)

@benmosher benmosher removed this from the 0.10.0 milestone Nov 9, 2015
@mctep
Copy link
Author

mctep commented Nov 9, 2015

Ok. I understand your reasons.

BTW. I has not found any plugin for resolving paths for require calls. So I can write it, but it will use a resolver similar to yours. And I think it will be pretty to add it feature in your module.

Maybe it will be a good decision to add an additional rule like no-unresolved-require.
So, please, look at this commit mctep@acd4b45

@mctep
Copy link
Author

mctep commented Nov 9, 2015

And support ArrayExpression for require arguments is not necessary, I think 😄

@benmosher
Copy link
Member

You make a strong case. Feels like it should be separate, though... Maybe as a setting?

I'll take another look. 😄

@benmosher
Copy link
Member

Check out my PR. I adapted your alternate rule into a commonjs setting.

Why were you supporting require([...]), though? for AMD? or something else? I'm not familiar with an array argument for CommonJS.

@mctep
Copy link
Author

mctep commented Nov 10, 2015

Oh. It's great! Thank you very much.

About an array of arguments. Yes it is using for AMD, and it should not be using for CommonJS. I realized it after commit only :-)

@benmosher
Copy link
Member

Added support for AMD via amd: true option, too. 😄 Thanks for your persistence!

@mctep
Copy link
Author

mctep commented Nov 10, 2015

🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants