From 815b43efeb283f27217f97eae50f3fe98831fc35 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Wed, 13 Apr 2016 20:44:26 +0200 Subject: [PATCH 1/4] Add `no-extraneous-dependencies` rule --- package.json | 5 +- src/core/importType.js | 39 ++++++++++ src/core/staticRequire.js | 7 ++ src/index.js | 1 + src/rules/no-extraneous-dependencies.js | 76 +++++++++++++++++++ tests/src/core/importType.js | 41 ++++++++++ tests/src/rules/no-extraneous-dependencies.js | 55 ++++++++++++++ 7 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 src/core/importType.js create mode 100644 src/core/staticRequire.js create mode 100644 src/rules/no-extraneous-dependencies.js create mode 100644 tests/src/core/importType.js create mode 100644 tests/src/rules/no-extraneous-dependencies.js diff --git a/package.json b/package.json index 46f078614..33acac1f4 100644 --- a/package.json +++ b/package.json @@ -65,11 +65,14 @@ "eslint": "2.x" }, "dependencies": { + "builtin-modules": "^1.1.1", "doctrine": "1.2.0", "es6-map": "^0.1.3", "es6-set": "^0.1.4", "es6-symbol": "*", "eslint-import-resolver-node": "^0.2.0", - "object-assign": "^4.0.1" + "lodash.cond": "^4.3.0", + "object-assign": "^4.0.1", + "pkg-up": "^1.0.0" } } diff --git a/src/core/importType.js b/src/core/importType.js new file mode 100644 index 000000000..0469433ad --- /dev/null +++ b/src/core/importType.js @@ -0,0 +1,39 @@ +'use strict' + +import cond from 'lodash.cond' +import builtinModules from 'builtin-modules' + +function constant(value) { + return () => value +} + +function isBuiltIn(name) { + return builtinModules.indexOf(name) !== -1 +} + +const externalModuleRegExp = /^\w/ +function isExternalModule(name) { + return externalModuleRegExp.test(name) +} + +function isRelativeToParent(name) { + return name.indexOf('../') === 0 +} + +const indexFiles = ['.', './', './index', './index.js'] +function isIndex(name) { + return indexFiles.indexOf(name) !== -1 +} + +function isRelativeToSibling(name) { + return name.indexOf('./') === 0 +} + +export default cond([ + [isBuiltIn, constant('builtin')], + [isExternalModule, constant('external')], + [isRelativeToParent, constant('parent')], + [isIndex, constant('index')], + [isRelativeToSibling, constant('sibling')], + [constant(true), constant('unknown')], +]) diff --git a/src/core/staticRequire.js b/src/core/staticRequire.js new file mode 100644 index 000000000..5a6596892 --- /dev/null +++ b/src/core/staticRequire.js @@ -0,0 +1,7 @@ +export default function isStaticRequire(node) { + return node && + node.callee.type === 'Identifier' && + node.callee.name === 'require' && + node.arguments.length === 1 && + node.arguments[0].type === 'Literal' +} diff --git a/src/index.js b/src/index.js index 90a8aeddf..7d1dd0502 100644 --- a/src/index.js +++ b/src/index.js @@ -13,6 +13,7 @@ export const rules = { 'no-amd': require('./rules/no-amd'), 'no-duplicates': require('./rules/no-duplicates'), 'imports-first': require('./rules/imports-first'), + 'no-extraneous-dependencies': require('./rules/no-extraneous-dependencies'), // metadata-based 'no-deprecated': require('./rules/no-deprecated'), diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js new file mode 100644 index 000000000..72b1b29dd --- /dev/null +++ b/src/rules/no-extraneous-dependencies.js @@ -0,0 +1,76 @@ +import fs from 'fs' +import pkgUp from 'pkg-up' +import importType from '../core/importType' +import isStaticRequire from '../core/staticRequire' + +function getDependencies() { + const filepath = pkgUp.sync() + if (!filepath) { + return null + } + + try { + const packageContent = JSON.parse(fs.readFileSync(filepath, 'utf8')) + return { + dependencies: packageContent.dependencies || {}, + devDependencies: packageContent.devDependencies || {}, + } + } catch (e) { + return null + } +} + +function missingErrorMessage(packageName) { + return `'${packageName}' is not listed in the project's dependencies. ` + + `Run 'npm i -S ${packageName}' to add it` +} + +function devDepErrorMessage(packageName) { + return `'${packageName}' is not listed in the project's dependencies, not devDependencies.` +} + +function reportIfMissing(context, deps, allowDevDeps, node, name) { + if (importType(name) !== 'external') { + return + } + const packageName = name.split('/')[0] + + if (deps.dependencies[packageName] === undefined) { + if (!allowDevDeps) { + context.report(node, devDepErrorMessage(packageName)) + } else if (deps.devDependencies[packageName] === undefined) { + context.report(node, missingErrorMessage(packageName)) + } + } +} + +module.exports = function (context) { + const options = context.options[0] || {} + const allowDevDeps = options.devDependencies !== false + const deps = getDependencies() + + if (!deps) { + return {} + } + + return { + ImportDeclaration: function (node) { + reportIfMissing(context, deps, allowDevDeps, node, node.source.value) + }, + CallExpression: function handleRequires(node) { + if (isStaticRequire(node)) { + reportIfMissing(context, deps, allowDevDeps, node, node.arguments[0].value) + } + }, + } +} + +module.exports.schema = [ + { + 'type': 'object', + 'properties': { + 'devDependencies': { 'type': 'boolean' }, + }, + 'additionalProperties': false, + }, +] diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js new file mode 100644 index 000000000..ad6547bae --- /dev/null +++ b/tests/src/core/importType.js @@ -0,0 +1,41 @@ +import { expect } from 'chai' +import importType from 'core/importType' + +describe('importType(name)', function () { + it("should return 'builtin' for node.js modules", function() { + expect(importType('fs')).to.equal('builtin') + expect(importType('path')).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') + }) + + 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') + }) + + 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') + }) + + 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') + }) + + it("should return 'unknown' for any unhandled cases", function() { + expect(importType('/malformed')).to.equal('unknown') + expect(importType(' foo')).to.equal('unknown') + }) +}) diff --git a/tests/src/rules/no-extraneous-dependencies.js b/tests/src/rules/no-extraneous-dependencies.js new file mode 100644 index 000000000..eb16cacbc --- /dev/null +++ b/tests/src/rules/no-extraneous-dependencies.js @@ -0,0 +1,55 @@ +import { test } from '../utils' + +import { RuleTester } from 'eslint' + +const ruleTester = new RuleTester() + , rule = require('rules/no-extraneous-dependencies') + +ruleTester.run('no-extraneous-dependencies', rule, { + valid: [ + test({ code: 'import "lodash.cond"'}), + test({ code: 'import "pkg-up"'}), + test({ code: 'import foo, { bar } from "lodash.cond"'}), + test({ code: 'import foo, { bar } from "pkg-up"'}), + test({ code: 'import "eslint"'}), + test({ code: 'import "eslint/lib/api"'}), + test({ code: 'require("lodash.cond")'}), + test({ code: 'require("pkg-up")'}), + test({ code: 'var foo = require("lodash.cond")'}), + test({ code: 'var foo = require("pkg-up")'}), + test({ code: 'import "fs"'}), + test({ code: 'import "./foo"'}), + ], + invalid: [ + test({ + code: 'import "not-a-dependency"', + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: '\'not-a-dependency\' is not listed in the project\'s dependencies. Run \'npm i -S not-a-dependency\' to add it', + }], + }), + test({ + code: 'import "eslint"', + options: [{devDependencies: false}], + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: '\'eslint\' is not listed in the project\'s dependencies, not devDependencies.', + }], + }), + test({ + code: 'var foo = require("not-a-dependency");', + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: '\'not-a-dependency\' is not listed in the project\'s dependencies. Run \'npm i -S not-a-dependency\' to add it', + }], + }), + test({ + code: 'var eslint = require("eslint");', + options: [{devDependencies: false}], + errors: [{ + ruleId: 'no-extraneous-dependencies', + message: '\'eslint\' is not listed in the project\'s dependencies, not devDependencies.', + }], + }), + ], +}) From bb8853a1e298d846bafa546705d58e66e9556375 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Thu, 14 Apr 2016 06:36:46 -0400 Subject: [PATCH 2/4] no-extraneous-dependencies: - lookup package.json relative to file being linted - added 'project' import type, which is ignored - uses resolved path to disambiguate some types of imports --- src/core/importType.js | 25 ++++++-- src/core/staticRequire.js | 1 + src/rules/no-extraneous-dependencies.js | 9 +-- tests/.eslintrc | 1 - tests/files/importType/index.js | 1 + tests/files/package.json | 14 ++++- tests/src/core/importType.js | 59 ++++++++++++------- tests/src/rules/no-extraneous-dependencies.js | 7 +++ 8 files changed, 85 insertions(+), 32 deletions(-) create mode 100644 tests/files/importType/index.js diff --git a/src/core/importType.js b/src/core/importType.js index 0469433ad..f74fda473 100644 --- a/src/core/importType.js +++ b/src/core/importType.js @@ -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 @@ -12,8 +13,14 @@ 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) { @@ -21,7 +28,8 @@ function isRelativeToParent(name) { } 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 } @@ -29,11 +37,16 @@ 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)) +} diff --git a/src/core/staticRequire.js b/src/core/staticRequire.js index 5a6596892..156dfeca4 100644 --- a/src/core/staticRequire.js +++ b/src/core/staticRequire.js @@ -1,3 +1,4 @@ +// todo: merge with module visitor export default function isStaticRequire(node) { return node && node.callee.type === 'Identifier' && diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index 72b1b29dd..f93ce12ce 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -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 } @@ -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] @@ -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) diff --git a/tests/.eslintrc b/tests/.eslintrc index 27d422055..700a3d688 100644 --- a/tests/.eslintrc +++ b/tests/.eslintrc @@ -3,5 +3,4 @@ env: mocha: true rules: no-unused-expressions: 0 - quotes: [2, 'single', 'avoid-escape'] max-len: 0 diff --git a/tests/files/importType/index.js b/tests/files/importType/index.js new file mode 100644 index 000000000..bc4ffafc8 --- /dev/null +++ b/tests/files/importType/index.js @@ -0,0 +1 @@ +/* for importType test, just needs to exist */ diff --git a/tests/files/package.json b/tests/files/package.json index 0a5fb6e23..1dea0d531 100644 --- a/tests/files/package.json +++ b/tests/files/package.json @@ -1 +1,13 @@ -{ "dummy": true } \ No newline at end of file +{ + "dummy": true, + "devDependencies": { + "eslint": "2.x" + }, + "peerDependencies": { + "eslint": "2.x" + }, + "dependencies": { + "lodash.cond": "^4.3.0", + "pkg-up": "^1.0.0" + } +} diff --git a/tests/src/core/importType.js b/tests/src/core/importType.js index ad6547bae..a86ef1a44 100644 --- a/tests/src/core/importType.js +++ b/tests/src/core/importType.js @@ -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') }) }) diff --git a/tests/src/rules/no-extraneous-dependencies.js b/tests/src/rules/no-extraneous-dependencies.js index eb16cacbc..edb80e6a2 100644 --- a/tests/src/rules/no-extraneous-dependencies.js +++ b/tests/src/rules/no-extraneous-dependencies.js @@ -1,4 +1,5 @@ import { test } from '../utils' +import * as path from 'path' import { RuleTester } from 'eslint' @@ -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({ From 84813157e44b0e76307d8a66b933e40afad52dd7 Mon Sep 17 00:00:00 2001 From: Ben Mosher Date: Thu, 14 Apr 2016 06:51:01 -0400 Subject: [PATCH 3/4] oh, node 0.x. includes => -1 ~ indexOf --- src/core/importType.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/importType.js b/src/core/importType.js index f74fda473..81b451444 100644 --- a/src/core/importType.js +++ b/src/core/importType.js @@ -15,12 +15,12 @@ function isBuiltIn(name) { const externalModuleRegExp = /^\w/ function isExternalModule(name, path) { if (!externalModuleRegExp.test(name)) return false - return (!path || path.includes(join('node_modules', name))) + return (!path || -1 < path.indexOf(join('node_modules', name))) } function isProjectModule(name, path) { if (!externalModuleRegExp.test(name)) return false - return (path && !path.includes(join('node_modules', name))) + return (path && -1 === path.indexOf(join('node_modules', name))) } function isRelativeToParent(name) { From 73ff01edbb0bf5db814af23acaa9af2f894149cc Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Mon, 18 Apr 2016 21:45:42 +0200 Subject: [PATCH 4/4] Add documentation for `no-extraneous-dependencies` --- CHANGELOG.md | 1 + README.md | 2 + docs/rules/no-extraneous-dependencies.md | 68 ++++++++++++++++++++++++ src/rules/no-extraneous-dependencies.js | 24 ++++----- 4 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 docs/rules/no-extraneous-dependencies.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b75f342e..d6d14b8ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ## [Unreleased] ### Added - [`no-named-as-default-member`] to `warnings` canned config +- add [`no-extraneous-dependencies`] rule ## [1.5.0] - 2016-04-18 ### Added diff --git a/README.md b/README.md index 3c785dd59..aedf3b689 100644 --- a/README.md +++ b/README.md @@ -48,10 +48,12 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a * Ensure all imports appear before other statements ([`imports-first`]) * Report repeated import of the same module in multiple places ([`no-duplicates`]) * Report namespace imports ([`no-namespace`]) +* Forbid the use of extraneous packages ([`no-extraneous-dependencies`]) [`imports-first`]: ./docs/rules/imports-first.md [`no-duplicates`]: ./docs/rules/no-duplicates.md [`no-namespace`]: ./docs/rules/no-namespace.md +[`no-extraneous-dependencies`]: ./docs/rules/no-extraneous-dependencies.md ## Installation diff --git a/docs/rules/no-extraneous-dependencies.md b/docs/rules/no-extraneous-dependencies.md new file mode 100644 index 000000000..e9be3afbb --- /dev/null +++ b/docs/rules/no-extraneous-dependencies.md @@ -0,0 +1,68 @@ +# Forbid the use of extraneous packages + +Forbid the import of external modules that are not declared in the `package.json`'s `dependencies` or `devDependencies`. +The closest parent `package.json` will be used. If no `package.json` is found, the rule will not lint anything. + +### Options + +This rule supports the following options: + +`devDependencies`: If set to `false`, then the rule will show an error when `devDependencies` are imported. Defaults to `true`. + +You can set the options like this: + +```js +"import/no-extraneous-dependencies": ["error", {"devDependencies": false}] +``` + + +## Rule Details + +Given the following `package.json`: +```json +{ + "name": "my-project", + "...": "...", + "dependencies": { + "builtin-modules": "^1.1.1", + "lodash.cond": "^4.2.0", + "lodash.find": "^4.2.0", + "pkg-up": "^1.0.0" + }, + "devDependencies": { + "ava": "^0.13.0", + "eslint": "^2.4.0", + "eslint-plugin-ava": "^1.3.0", + "xo": "^0.13.0" + } +} +``` + + +## Fail + +```js +var _ = require('lodash'); +import _ from 'lodash'; + +/* eslint import/no-extraneous-dependencies: ["error", {"devDependencies": false}] */ +import test from 'ava'; +var test = require('ava'); +``` + + +## Pass + +```js +// Builtin and internal modules are fine +var path = require('path'); +var foo = require('./foo'); + +import test from 'ava'; +import find from 'lodash.find'; +``` + + +## When Not To Use It + +If you do not have a `package.json` file in your project. diff --git a/src/rules/no-extraneous-dependencies.js b/src/rules/no-extraneous-dependencies.js index f93ce12ce..9027afdf3 100644 --- a/src/rules/no-extraneous-dependencies.js +++ b/src/rules/no-extraneous-dependencies.js @@ -5,28 +5,28 @@ import isStaticRequire from '../core/staticRequire' function getDependencies(context) { const filepath = pkgUp.sync(context.getFilename()) - if (!filepath) { - return null - } + if (!filepath) { + return null + } - try { - const packageContent = JSON.parse(fs.readFileSync(filepath, 'utf8')) - return { + try { + const packageContent = JSON.parse(fs.readFileSync(filepath, 'utf8')) + return { dependencies: packageContent.dependencies || {}, devDependencies: packageContent.devDependencies || {}, } - } catch (e) { - return null - } + } catch (e) { + return null + } } function missingErrorMessage(packageName) { - return `'${packageName}' is not listed in the project's dependencies. ` + - `Run 'npm i -S ${packageName}' to add it` + return `'${packageName}' is not listed in the project's dependencies. ` + + `Run 'npm i -S ${packageName}' to add it` } function devDepErrorMessage(packageName) { - return `'${packageName}' is not listed in the project's dependencies, not devDependencies.` + return `'${packageName}' is not listed in the project's dependencies, not devDependencies.` } function reportIfMissing(context, deps, allowDevDeps, node, name) {