Skip to content

Commit

Permalink
no-extraneous-dependencies:
Browse files Browse the repository at this point in the history
- lookup package.json relative to file being linted
- added 'project' import type, which is ignored
- uses resolved path to disambiguate some types of imports
  • Loading branch information
benmosher committed Apr 14, 2016
1 parent b3ac0f8 commit 266eb18
Show file tree
Hide file tree
Showing 8 changed files with 85 additions and 32 deletions.
25 changes: 19 additions & 6 deletions src/core/importType.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
'use strict'

import cond from 'lodash.cond'
import builtinModules from 'builtin-modules'
import { basename, join } from 'path'

import resolve from './resolve'

function constant(value) {
return () => value
Expand All @@ -12,28 +13,40 @@ function isBuiltIn(name) {
}

const externalModuleRegExp = /^\w/
function isExternalModule(name) {
return externalModuleRegExp.test(name)
function isExternalModule(name, path) {
if (!externalModuleRegExp.test(name)) return false
return (!path || path.includes(join('node_modules', name)))
}

function isProjectModule(name, path) {
if (!externalModuleRegExp.test(name)) return false
return (path && !path.includes(join('node_modules', name)))
}

function isRelativeToParent(name) {
return name.indexOf('../') === 0
}

const indexFiles = ['.', './', './index', './index.js']
function isIndex(name) {
function isIndex(name, path) {
if (path) return basename(path).split('.')[0] === 'index'
return indexFiles.indexOf(name) !== -1
}

function isRelativeToSibling(name) {
return name.indexOf('./') === 0
}

export default cond([
const typeTest = cond([
[isBuiltIn, constant('builtin')],
[isExternalModule, constant('external')],
[isProjectModule, constant('project')],
[isRelativeToParent, constant('parent')],
[isIndex, constant('index')],
[isRelativeToSibling, constant('sibling')],
[constant(true), constant('unknown')],
])

export default function resolveImportType(name, context) {
return typeTest(name, resolve(name, context))
}
1 change: 1 addition & 0 deletions src/core/staticRequire.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// todo: merge with module visitor
export default function isStaticRequire(node) {
return node &&
node.callee.type === 'Identifier' &&
Expand Down
9 changes: 5 additions & 4 deletions src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import pkgUp from 'pkg-up'
import importType from '../core/importType'
import isStaticRequire from '../core/staticRequire'

function getDependencies() {
const filepath = pkgUp.sync()
function getDependencies(context) {
const filepath = pkgUp.sync(context.getFilename())
if (!filepath) {
return null
}
Expand All @@ -30,7 +30,7 @@ function devDepErrorMessage(packageName) {
}

function reportIfMissing(context, deps, allowDevDeps, node, name) {
if (importType(name) !== 'external') {
if (importType(name, context) !== 'external') {
return
}
const packageName = name.split('/')[0]
Expand All @@ -47,12 +47,13 @@ function reportIfMissing(context, deps, allowDevDeps, node, name) {
module.exports = function (context) {
const options = context.options[0] || {}
const allowDevDeps = options.devDependencies !== false
const deps = getDependencies()
const deps = getDependencies(context)

if (!deps) {
return {}
}

// todo: use module visitor from module-utils core
return {
ImportDeclaration: function (node) {
reportIfMissing(context, deps, allowDevDeps, node, node.source.value)
Expand Down
1 change: 0 additions & 1 deletion tests/.eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,4 @@ env:
mocha: true
rules:
no-unused-expressions: 0
quotes: [2, 'single', 'avoid-escape']
max-len: 0
1 change: 1 addition & 0 deletions tests/files/importType/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/* for importType test, just needs to exist */
14 changes: 13 additions & 1 deletion tests/files/package.json
Original file line number Diff line number Diff line change
@@ -1 +1,13 @@
{ "dummy": true }
{
"dummy": true,
"devDependencies": {
"eslint": "2.x"
},
"peerDependencies": {
"eslint": "2.x"
},
"dependencies": {
"lodash.cond": "^4.3.0",
"pkg-up": "^1.0.0"
}
}
59 changes: 39 additions & 20 deletions tests/src/core/importType.js
Original file line number Diff line number Diff line change
@@ -1,41 +1,60 @@
import { expect } from 'chai'
import * as path from 'path'

import importType from 'core/importType'

import { testContext } from '../utils'

describe('importType(name)', function () {
const context = testContext()

it("should return 'builtin' for node.js modules", function() {
expect(importType('fs')).to.equal('builtin')
expect(importType('path')).to.equal('builtin')
expect(importType('fs', context)).to.equal('builtin')
expect(importType('path', context)).to.equal('builtin')
})

it("should return 'external' for non-builtin modules without a relative path", function() {
expect(importType('lodash')).to.equal('external')
expect(importType('async')).to.equal('external')
expect(importType('chalk')).to.equal('external')
expect(importType('foo')).to.equal('external')
expect(importType('lodash.find')).to.equal('external')
expect(importType('lodash/fp')).to.equal('external')
expect(importType('lodash', context)).to.equal('external')
expect(importType('async', context)).to.equal('external')
expect(importType('chalk', context)).to.equal('external')
expect(importType('foo', context)).to.equal('external')
expect(importType('lodash.find', context)).to.equal('external')
expect(importType('lodash/fp', context)).to.equal('external')
})

it("should return 'project' for non-builtins resolved outside of node_modules", function () {
const pathContext = testContext({ "import/resolver": { node: { paths: [ path.join(__dirname, '..', '..', 'files') ] } } })
expect(importType('importType', pathContext)).to.equal('project')
})

it("should return 'parent' for internal modules that go through the parent", function() {
expect(importType('../foo')).to.equal('parent')
expect(importType('../../foo')).to.equal('parent')
expect(importType('../bar/foo')).to.equal('parent')
expect(importType('../foo', context)).to.equal('parent')
expect(importType('../../foo', context)).to.equal('parent')
expect(importType('../bar/foo', context)).to.equal('parent')
})

it("should return 'sibling' for internal modules that are connected to one of the siblings", function() {
expect(importType('./foo')).to.equal('sibling')
expect(importType('./foo/bar')).to.equal('sibling')
expect(importType('./foo', context)).to.equal('sibling')
expect(importType('./foo/bar', context)).to.equal('sibling')
})

it("should return 'index' for sibling index file", function() {
expect(importType('.')).to.equal('index')
expect(importType('./')).to.equal('index')
expect(importType('./index')).to.equal('index')
expect(importType('./index.js')).to.equal('index')
describe("should return 'index' for sibling index file when", function() {
it("resolves", function() {
expect(importType('./importType', context)).to.equal('index')
expect(importType('./importType/', context)).to.equal('index')
expect(importType('./importType/index', context)).to.equal('index')
expect(importType('./importType/index.js', context)).to.equal('index')
})
it("doesn't resolve", function() {
expect(importType('.', context)).to.equal('index')
expect(importType('./', context)).to.equal('index')
expect(importType('./index', context)).to.equal('index')
expect(importType('./index.js', context)).to.equal('index')
})
})

it("should return 'unknown' for any unhandled cases", function() {
expect(importType('/malformed')).to.equal('unknown')
expect(importType(' foo')).to.equal('unknown')
expect(importType('/malformed', context)).to.equal('unknown')
expect(importType(' foo', context)).to.equal('unknown')
})
})
7 changes: 7 additions & 0 deletions tests/src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { test } from '../utils'
import * as path from 'path'

import { RuleTester } from 'eslint'

Expand All @@ -19,6 +20,12 @@ ruleTester.run('no-extraneous-dependencies', rule, {
test({ code: 'var foo = require("pkg-up")'}),
test({ code: 'import "fs"'}),
test({ code: 'import "./foo"'}),

// 'project' type
test({
code: 'import "importType"',
settings: { "import/resolver": { node: { paths: [ path.join(__dirname, '../../files') ] } } },
}),
],
invalid: [
test({
Expand Down

0 comments on commit 266eb18

Please sign in to comment.