Skip to content

Commit

Permalink
no-extraneous-dependencies: add support/option for `peerDependencie…
Browse files Browse the repository at this point in the history
…s` (#423)
  • Loading branch information
jfmengels committed Jul 9, 2016
1 parent ea84f34 commit 2da02a7
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 11 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## [Unreleased]
### Added
- Added an `peerDependencies` option to [`no-extraneous-dependencies`] to allow/forbid peer dependencies ([#423], [#428]).

### Fixed
- removing `Symbol` dependencies (i.e. `for-of` loops) due to Node 0.10 polyfill
issue (see [#415]). Should not make any discernible semantic difference.
Expand Down Expand Up @@ -249,6 +252,7 @@ for info on changes for earlier releases.
[`prefer-default-export`]: ./docs/rules/prefer-default-export.md
[`no-restricted-paths`]: ./docs/rules/no-restricted-paths.md

[#428]: https://github.com/benmosher/eslint-plugin-import/pull/428
[#395]: https://github.com/benmosher/eslint-plugin-import/pull/395
[#371]: https://github.com/benmosher/eslint-plugin-import/pull/371
[#365]: https://github.com/benmosher/eslint-plugin-import/pull/365
Expand Down Expand Up @@ -282,6 +286,7 @@ for info on changes for earlier releases.
[#157]: https://github.com/benmosher/eslint-plugin-import/pull/157
[#314]: https://github.com/benmosher/eslint-plugin-import/pull/314

[#423]: https://github.com/benmosher/eslint-plugin-import/issues/423
[#415]: https://github.com/benmosher/eslint-plugin-import/issues/415
[#386]: https://github.com/benmosher/eslint-plugin-import/issues/386
[#373]: https://github.com/benmosher/eslint-plugin-import/issues/373
Expand Down
13 changes: 11 additions & 2 deletions docs/rules/no-extraneous-dependencies.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Forbid the use of extraneous packages

Forbid the import of external modules that are not declared in the `package.json`'s `dependencies` or `devDependencies`.
Forbid the import of external modules that are not declared in the `package.json`'s `dependencies`, `devDependencies`, `optionalDependencies` or `peerDependencies`.
The closest parent `package.json` will be used. If no `package.json` is found, the rule will not lint anything.

### Options
Expand All @@ -9,11 +9,12 @@ 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`.
`optionalDependencies`: If set to `false`, then the rule will show an error when `optionalDependencies` are imported. Defaults to `true`.
`peerDependencies`: If set to `false`, then the rule will show an error when `peerDependencies` are imported. Defaults to `false`.

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 10, 2016

Member

I'm having second thoughts on defaulting this to false, since the other two are defaulted to true. Probably makes sense to default to true for consistency.

What do you think?

This comment has been minimized.

Copy link
@jfmengels

jfmengels Jul 10, 2016

Author Collaborator

I think we should default to whatever makes the user avoid the most errors. If you think that there won't be a lot of errors missed with true by default, or that the benefit doesn't outweigh the inconsistency, I don't mind changing it.

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 10, 2016

Member

Yeah, upon reflection I can't think of a reason anyone would declare a peer dep and then want to have the linter complain about it.

In the React example, you definitely want to use it in code and often won't have it also as a dev dep.

So true is probably a better default for users, too.

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 10, 2016

Member

Which is to say: probably little to no benefit from defaulting to false.

Might not even warrant the option, honestly, but since the other two types are configurable, it doesn't hurt.


You can set the options like this:

```js
"import/no-extraneous-dependencies": ["error", {"devDependencies": false, "optionalDependencies": false}]
"import/no-extraneous-dependencies": ["error", {"devDependencies": false, "optionalDependencies": false, "peerDependencies": false}]
```


Expand All @@ -38,6 +39,9 @@ Given the following `package.json`:
},
"optionalDependencies": {
"lodash.isarray": "^4.0.0"
},
"peerDependencies": {
"react": ">=15.0.0 <16.0.0"
}
}
```
Expand All @@ -49,6 +53,8 @@ Given the following `package.json`:
var _ = require('lodash');
import _ from 'lodash';

import react from 'react';

/* eslint import/no-extraneous-dependencies: ["error", {"devDependencies": false}] */
import test from 'ava';
var test = require('ava');
Expand All @@ -69,6 +75,9 @@ var foo = require('./foo');
import test from 'ava';
import find from 'lodash.find';
import find from 'lodash.isarray';

/* eslint import/no-extraneous-dependencies: ["error", {"peerDependencies": true}] */
import react from 'react';
```


Expand Down
26 changes: 17 additions & 9 deletions src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ function getDependencies(context) {
dependencies: packageContent.dependencies || {},
devDependencies: packageContent.devDependencies || {},
optionalDependencies: packageContent.optionalDependencies || {},
peerDependencies: packageContent.peerDependencies || {},
}
} catch (e) {
return null
Expand All @@ -35,7 +36,7 @@ function optDepErrorMessage(packageName) {
`not optionalDependencies.`
}

function reportIfMissing(context, deps, allowDevDeps, allowOptDeps, node, name) {
function reportIfMissing(context, deps, depsOptions, node, name) {
if (importType(name, context) !== 'external') {
return
}
Expand All @@ -47,20 +48,22 @@ function reportIfMissing(context, deps, allowDevDeps, allowOptDeps, node, name)
const isInDeps = deps.dependencies[packageName] !== undefined
const isInDevDeps = deps.devDependencies[packageName] !== undefined
const isInOptDeps = deps.optionalDependencies[packageName] !== undefined
const isInPeerDeps = deps.peerDependencies[packageName] === undefined

This comment has been minimized.

Copy link
@benmosher

benmosher Jul 10, 2016

Member

Ah, so this is === to allow a false default value? Seems like another consistency vote for defaulting to true after all. I'm not sure what I was thinking...


if (isInDeps ||
(allowDevDeps && isInDevDeps) ||
(allowOptDeps && isInOptDeps)
(depsOptions.allowDevDeps && isInDevDeps) ||
(depsOptions.allowPeerDeps && isInPeerDeps) ||
(depsOptions.allowOptDeps && isInOptDeps)
) {
return
}

if (isInDevDeps && !allowDevDeps) {
if (isInDevDeps && !depsOptions.allowDevDeps) {
context.report(node, devDepErrorMessage(packageName))
return
}

if (isInOptDeps && !allowOptDeps) {
if (isInOptDeps && !depsOptions.allowOptDeps) {
context.report(node, optDepErrorMessage(packageName))
return
}
Expand All @@ -70,22 +73,26 @@ function reportIfMissing(context, deps, allowDevDeps, allowOptDeps, node, name)

module.exports = function (context) {
const options = context.options[0] || {}
const allowDevDeps = options.devDependencies !== false
const allowOptDeps = options.optionalDependencies !== false
const deps = getDependencies(context)

if (!deps) {
return {}
}

const depsOptions = {
allowDevDeps: options.devDependencies !== false,
allowOptDeps: options.optionalDependencies !== false,
allowPeerDeps: options.peerDependencies === true,
}

// todo: use module visitor from module-utils core
return {
ImportDeclaration: function (node) {
reportIfMissing(context, deps, allowDevDeps, allowOptDeps, node, node.source.value)
reportIfMissing(context, deps, depsOptions, node, node.source.value)
},
CallExpression: function handleRequires(node) {
if (isStaticRequire(node)) {
reportIfMissing(context, deps, allowDevDeps, allowOptDeps, node, node.arguments[0].value)
reportIfMissing(context, deps, depsOptions, node, node.arguments[0].value)
}
},
}
Expand All @@ -97,6 +104,7 @@ module.exports.schema = [
'properties': {
'devDependencies': { 'type': 'boolean' },
'optionalDependencies': { 'type': 'boolean' },
'peerDependencies': { 'type': 'boolean' },
},
'additionalProperties': false,
},
Expand Down
13 changes: 13 additions & 0 deletions tests/src/rules/no-extraneous-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ ruleTester.run('no-extraneous-dependencies', rule, {
test({ code: 'import "@scope/core"'}),

test({ code: 'import "electron"', settings: { 'import/core-modules': ['electron'] } }),
test({
code: 'import "eslint"',
options: [{peerDependencies: true}],
settings: { 'import/resolver': { node: { paths: [ path.join(__dirname, '../../files') ] } } },
}),

// 'project' type
test({
Expand Down Expand Up @@ -84,6 +89,14 @@ ruleTester.run('no-extraneous-dependencies', rule, {
message: '\'eslint\' should be listed in the project\'s dependencies, not devDependencies.',
}],
}),
test({
code: 'var eslint = require("eslint")',
options: [{devDependencies: false, peerDependencies: false}],
errors: [{
ruleId: 'no-extraneous-dependencies',
message: '\'eslint\' should be listed in the project\'s dependencies, not devDependencies.',
}],
}),
test({
code: 'var eslint = require("lodash.isarray")',
options: [{optionalDependencies: false}],
Expand Down

0 comments on commit 2da02a7

Please sign in to comment.