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

Add no-extraneous-dependencies rule #9

Closed
wants to merge 1 commit into from

Conversation

jfmengels
Copy link
Owner

Adds no-extraneous-dependencies rule

As noted import-js/eslint-plugin-import#126 (comment) (under Regarding resolution) here, some additional work needs to be done to ignore internal modules when accessed with external module-like paths (see the comment for an example).

As that seems to me like it's not a wide-spread practice, I will create an issue to implement that later. For now, this should suit the common case.

@kentcdodds @benmosher If you have some feedback for this, I'd appreciate it :)

I'm not sure whether this will land in the eslint-plugin-import-order or if I create a new package/fork of the repo with a more generic name (eslint-plugin-import-rules... ? :s), cc @sindresorhus.

@kentcdodds
Copy link

This looks good to me! Great work! Though this plugin definitely doesn't seem to be the proper place to put this rule. Would make a lot more sense in the import plugin IMO.

@jfmengels
Copy link
Owner Author

Would make a lot more sense in the import plugin IMO.

Agreed, but @benmosher doesn't want it there (or rather, sees it better fit for an other project).

I don't mind creating a new project for it (eslint-plugin-npm might be a good fit), but I'm starting to have plenty of them...
image
(okay, some are forks to which I have not contributed, but hey, dramatic effect).

What do you think? Shall I create eslint-plugin-npm?

@benmosher
Copy link

Agreed, but @benmosher doesn't want it there (or rather, sees it better fit for an other project).

Sorry, I think I left a bunch of backstory out. I'm not against it being in the import plugin, I just haven't merged my PR to actually use eslint-module-utils in the import plugin. I've just been goofing around with it off in eslint-plugin-git land. I think this rule would be a perfect application of the stuff I've been building in eslint-module-utils, though.

So my dream implementation can't be created in eslint-plugin-import at this exact moment. But I'm up for merging this in if you think it makes sense there. I think I'm leaning that way as well, probably better to not force folks to install one more plugin for just this rule.

@jfmengels
Copy link
Owner Author

probably better to not force folks to install one more plugin for just this rule

👍

I started working on this in your repo, and haven't deleted it. I don't mind making a PR for it. Let me know :) If so, let me know if you have feedback about my implementation.

Also, if you want to include the import-order rule, I'm all ears. I think it's a better fit there that in this single rule repo (cc @sindresorhus @jamestalmage who maintain XO and wo rely on this plugin).

@benmosher
Copy link

Let's do the PR. I'm not sure when I'll be able to get to detailed feedback, but let's get it opened over there so we can banter on it 😄

As for import-order: I am up for merging it into import plugin.

I was planning to drop support for Node less than 4.x soon, though; is that important for you? ESLint is planning to do this sometime soon as well, but I'm not sure when they'll get around to it.

@satya164
Copy link

Yeah, makes more sense to have it in eslint-plugin-import.

Great job @jfmengels <3

@jfmengels
Copy link
Owner Author

Cool! Will make the PR one of these days, and probably another one for import-order in the same go.

(bye bye my few GH stars and npm downloads... :p)

(I'll close this once I made the PR, in case people want to give feedback)

@jfmengels
Copy link
Owner Author

Closing as it landed in eslint-plugin-import

@jfmengels jfmengels closed this Apr 19, 2016
@jfmengels jfmengels deleted the no-extraneous-dependencies branch April 19, 2016 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants