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 #241

Merged
merged 4 commits into from
Apr 19, 2016

Conversation

jfmengels
Copy link
Collaborator

Add no-extraneous-dependencies rule

(kinda) Continuous of jfmengels/eslint-plugin-import-order#9.

I've added two files : core/importType and core/staticRequire. I use those in import-order for the import-order` rule, and will also use them in the no builtin rule probably.

Let me know if there's anything I need to change :)
(If this gets merged, I'll get started on the PRs for import-order and no builtin, but this reuses some of the same code)

@benmosher
Copy link
Member

benmosher commented Apr 14, 2016

There are a few todos blocked on #234 merging into master, but I made some changes and pushed to another branch: 266eb18

Thought it might be simpler to write the code than the English 😅

Fundamentally, I just added a project import type and use the resolved path to distinguish the two.

I also changed the pkgUp call to use the filepath for the file being linted instead of the CWD of the lint process. This makes a difference for large projects with nested modules, and also gives you a bit more wiggle room for running ESLint from anywhere instead of just inside the project tree.

We may want to add a cache for the resolved package dependencies, but I think there is goodness in #234 that will make that easier too.

I'd also like to get AMD support via #234, but that shouldn't block merging + deploying this.

Things that need to get done either way:

  • docs/rules/no-extraneous-dependencies.md
  • changelog blurb
  • README blurb

@benmosher
Copy link
Member

BTW I just made you a collaborator, figured that would make this and the import-order merge simpler. 😄

@jfmengels
Copy link
Collaborator Author

I just made you a collaborator

Thanks!

I made some changes and pushed to another branch: 266eb18

Should I make a new PR with that new branch to discuss stuff there, or do you want to wait until #234 lands? Sounds like a good idea to me.

I also changed the pkgUp call to use the filepath for the file being linted

Ah, that sounds like a good idea yeah! 👍

Things that need to get done either way (README...)

My bad, I did them in my project, forgot to do that here. Will do that some time later.

I noticed you changed the index type, so that ./importType gets considered as an index file if it points to an index file.
I don't agree with this change. The name was maybe badly chosen, but that was not the original purpose. The reason I wrote those types was to be able to order imports in import-order (see sindresorhus/project-ideas#62 for the original request).
To me, an index file is the index file of the current directory. Anything else, may it point to an index file or not, is not an index.
The whole "beauty" IMO when doing import './bar' is that you don't care if it actually points to './bar/index.js or to ./bar.js, which I find really useful (so much that I like to write a lot of index files, which then allows me to scope the internal methods of the module in other files under ./bar).

Given the following directory:

  • foo/
    • bar/
      • baz/
        • index.js
      • index.js
      • a.js (current file)
    • index.js
// foo/bar/a.js
import './baz' // sibling
import './baz/index.js' // sibling
import '../' // parent

import './' // index
import './index' // index

In short, ./importType, as in your test examples, should be a sibling, not an index.

@benmosher
Copy link
Member

New PR: I think that makes sense, we can hack on it together in this repo. I'm not sure when I'll get a chance to actually do the module-utils stuff (probably not sooner than Tuesday), so you can open the PR whenever feels right.

On index type: that all makes sense, I definitely misunderstood the intent.

It actually probably makes sense to go ahead and merge and ship the first version of this. I don't think the index mischaracterization impacts this rule, and it is already very useful. I don't see a reason to block on anything but the Changelog and a little README blurb--could even put it in the "experimental" block if we're feeling like it might break.

No rush, though, we can just merge it as soon as some minimum of documentation is in place (if you concur).

@benmosher
Copy link
Member

FYI: just flushed the other pending changes to npm with v1.5.0, ready to ship this as soon as docs are ready (I just didn't have quite enough time to do that and get the rest shipped this morning)

@jfmengels
Copy link
Collaborator Author

Sure, will try to add this tonight!

jfmengels and others added 3 commits April 18, 2016 21:45
- lookup package.json relative to file being linted
- added 'project' import type, which is ignored
- uses resolved path to disambiguate some types of imports
@jfmengels
Copy link
Collaborator Author

jfmengels commented Apr 18, 2016

I have updated this branch to use what you've made in the branch of this repo. I've added documentation to your branch (and rebased the whole thing).
Let me know if there's anything else to change!

@jfmengels jfmengels mentioned this pull request Apr 18, 2016
4 tasks
@benmosher
Copy link
Member

Thanks, looks good!

I need to undo the breaking change I made to the 'index' type, but I'll do that post merge.

@benmosher benmosher merged commit 20b6bbc into import-js:master Apr 19, 2016
@jfmengels
Copy link
Collaborator Author

Cool!
For the index type, I undid it in the import-order PR.

@benmosher
Copy link
Member

Ah, got it. I should have looked there first, had just undone it in master locally. I'll dump my commit and merge yours.

@jfmengels jfmengels deleted the no-extraneous-dependencies branch April 19, 2016 11:14
@benmosher
Copy link
Member

Experiment: closes #126

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

Successfully merging this pull request may close these issues.

2 participants