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 allow option to no-nodejs-modules rule (fixes #452) #509

Merged
merged 3 commits into from
Aug 23, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).

## [Unreleased]

- Added an `allow` option to [`no-nodejs-modules`] to allow exceptions ([#452], [#509]).

## [1.14.0] - 2016-08-22
### Added
Expand Down Expand Up @@ -290,6 +290,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

[#509]: https://github.com/benmosher/eslint-plugin-import/pull/509
[#503]: https://github.com/benmosher/eslint-plugin-import/pull/503
[#499]: https://github.com/benmosher/eslint-plugin-import/pull/499
[#461]: https://github.com/benmosher/eslint-plugin-import/pull/461
Expand Down Expand Up @@ -331,6 +332,7 @@ for info on changes for earlier releases.
[#478]: https://github.com/benmosher/eslint-plugin-import/issues/478
[#456]: https://github.com/benmosher/eslint-plugin-import/issues/456
[#453]: https://github.com/benmosher/eslint-plugin-import/issues/453
[#452]: https://github.com/benmosher/eslint-plugin-import/issues/452
[#441]: https://github.com/benmosher/eslint-plugin-import/issues/441
[#423]: https://github.com/benmosher/eslint-plugin-import/issues/423
[#416]: https://github.com/benmosher/eslint-plugin-import/issues/416
Expand Down
9 changes: 9 additions & 0 deletions docs/rules/no-nodejs-modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

Forbid the use of Node.js builtin modules. Can be useful for client-side web projects that do not have access to those modules.

### Options

This rule supports the following options:

- `allow`: Array of names of allowed modules. Defaults to an empty array.

## Rule Details

### Fail
Expand All @@ -24,6 +30,9 @@ import foo from './foo';
var _ = require('lodash');
var foo = require('foo');
var foo = require('./foo');

/* eslint import/no-nodejs-modules: ["error", {"allow": ["path"]}] */
import path from 'path';
```

## When Not To Use It
Expand Down
11 changes: 7 additions & 4 deletions src/rules/no-nodejs-modules.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
import importType from '../core/importType'
import isStaticRequire from '../core/staticRequire'

function reportIfMissing(context, node, name) {
if (importType(name, context) === 'builtin') {
function reportIfMissing(context, node, allowed, name) {
if (allowed.indexOf(name) === -1 && importType(name, context) === 'builtin') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, bummer, I also thought Node v4 had includes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that surprising actually, since it's not even ES2015, but an ES2016 feature. I thought Babel would actually take care of this, but I guess was too hopeful (or it's just something to add). Anyway indexOf is fine.

context.report(node, 'Do not import Node.js builtin module "' + name + '"')
}
}

module.exports = function (context) {
const options = context.options[0] || {}
const allowed = options.allow || []

return {
ImportDeclaration: function handleImports(node) {
reportIfMissing(context, node, node.source.value)
reportIfMissing(context, node, allowed, node.source.value)
},
CallExpression: function handleRequires(node) {
if (isStaticRequire(node)) {
reportIfMissing(context, node, node.arguments[0].value)
reportIfMissing(context, node, allowed, node.arguments[0].value)
}
},
}
Expand Down
37 changes: 37 additions & 0 deletions tests/src/rules/no-nodejs-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,36 @@ ruleTester.run('no-nodejs-modules', rule, {
test({ code: 'var foo = require("foo")'}),
test({ code: 'var foo = require("./")'}),
test({ code: 'var foo = require("@scope/foo")'}),
test({
code: 'import events from "events"',
options: [{
allow: ['events'],
}],
}),
test({
code: 'import path from "path"',
options: [{
allow: ['path'],
}],
}),
test({
code: 'var events = require("events")',
options: [{
allow: ['events'],
}],
}),
test({
code: 'var path = require("path")',
options: [{
allow: ['path'],
}],
}),
test({
code: 'import path from "path";import events from "events"',
options: [{
allow: ['path', 'events'],
}],
}),
],
invalid: [
test({
Expand All @@ -44,5 +74,12 @@ ruleTester.run('no-nodejs-modules', rule, {
code: 'var fs = require("fs")',
errors: [error('Do not import Node.js builtin module "fs"')],
}),
test({
code: 'import fs from "fs"',
options: [{
allow: ['path'],
}],
errors: [error('Do not import Node.js builtin module "fs"')],
}),
],
})