Skip to content

Commit

Permalink
Add no-dynamic-require rule (fixes #567)
Browse files Browse the repository at this point in the history
  • Loading branch information
jfmengels committed Sep 20, 2016
1 parent 0f527d2 commit 91b2de2
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 0 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel

## [Unreleased]
### Added
- New rule [`no-dynamic-require`]: restrict deep package imports to specific folders. ([#567])
- New rule [`no-internal-modules`]: restrict deep package imports to specific folders. ([#485], thanks [@spalger]!)
- [`extensions`]: allow override of a chosen default with options object ([#555], thanks [@ljharb]!)

Expand Down Expand Up @@ -311,7 +312,9 @@ for info on changes for earlier releases.
[`no-absolute-path`]: ./docs/rules/no-absolute-path.md
[`max-dependencies`]: ./docs/rules/max-dependencies.md
[`no-internal-modules`]: ./docs/rules/no-internal-modules.md
[`no-dynamic-require`]: ./docs/rules/no-dynamic-require.md

[#567]: https://github.com/benmosher/eslint-plugin-import/pull/567
[#555]: https://github.com/benmosher/eslint-plugin-import/pull/555
[#538]: https://github.com/benmosher/eslint-plugin-import/pull/538
[#527]: https://github.com/benmosher/eslint-plugin-import/pull/527
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Ensure imported namespaces contain dereferenced properties as they are dereferenced. ([`namespace`])
* Restrict which files can be imported in a given folder ([`no-restricted-paths`])
* Forbid import of modules using absolute paths ([`no-absolute-path`])
* Forbid `require()` calls with expressions ([`no-dynamic-require`])

[`no-unresolved`]: ./docs/rules/no-unresolved.md
[`named`]: ./docs/rules/named.md
[`default`]: ./docs/rules/default.md
[`namespace`]: ./docs/rules/namespace.md
[`no-restricted-paths`]: ./docs/rules/no-restricted-paths.md
[`no-absolute-path`]: ./docs/rules/no-absolute-path.md
[`no-dynamic-require`]: ./docs/rules/no-dynamic-require.md

**Helpful warnings:**

Expand Down
24 changes: 24 additions & 0 deletions docs/rules/no-dynamic-require.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Forbid `require()` calls with expressions

The `require` method from CommonJS is used to import modules from different files. Unlike the ES6 `import` syntax, it can be given expressions that will be resolved at runtime. While this is sometimes necessary and useful, in most cases it isn't. Using expressions (for instance, concatenating a path and variable) as the argument makes it harder for tools to do static code analysis, or to find where in the codebase a module is used.

This rule checks every call to `require()` that uses expressions for the module name argument.

## Rule Details

### Fail

```js
require(name);
require('../' + name);
require(`../${name}`);
require(name());
```

### Pass

```js
require('../name');
require('../name' + name);
require(`../name`);
```
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const rules = {
'order': require('./rules/order'),
'newline-after-import': require('./rules/newline-after-import'),
'prefer-default-export': require('./rules/prefer-default-export'),
'no-dynamic-require': require('./rules/no-dynamic-require'),

// metadata-based
'no-deprecated': require('./rules/no-deprecated'),
Expand Down
25 changes: 25 additions & 0 deletions src/rules/no-dynamic-require.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
function isRequire(node) {
return node &&
node.callee &&
node.callee.type === 'Identifier' &&
node.callee.name === 'require' &&
node.arguments.length >= 1
}

function isStaticValue(arg) {
return arg.type === 'Literal' ||
(arg.type === 'TemplateLiteral' && arg.expressions.length === 0)
}

module.exports = function (context) {
return {
CallExpression(node) {
if (isRequire(node) && !isStaticValue(node.arguments[0])) {
context.report({
node,
message: 'Calls to require() should use string literals',
})
}
},
}
}
49 changes: 49 additions & 0 deletions tests/src/rules/no-dynamic-require.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { test } from '../utils'

import { RuleTester } from 'eslint'

const ruleTester = new RuleTester()
, rule = require('rules/no-dynamic-require')

const error = {
ruleId: 'no-dynamic-require',
message: 'Calls to require() should use string literals',
}

ruleTester.run('no-dynamic-require', rule, {
valid: [
test({ code: 'import _ from "lodash"'}),
test({ code: 'require("foo")'}),
test({ code: 'require(`foo`)'}),
test({ code: 'require("./foo")'}),
test({ code: 'require("@scope/foo")'}),
test({ code: 'require()'}),
test({ code: 'require("./foo", "bar" + "okay")'}),
test({ code: 'var foo = require("foo")'}),
test({ code: 'var foo = require(`foo`)'}),
test({ code: 'var foo = require("./foo")'}),
test({ code: 'var foo = require("@scope/foo")'}),
],
invalid: [
test({
code: 'require("../" + name)',
errors: [error],
}),
test({
code: 'require(`../${name}`)',
errors: [error],
}),
test({
code: 'require(name)',
errors: [error],
}),
test({
code: 'require(name())',
errors: [error],
}),
test({
code: 'require(name + "foo", "bar")',
errors: [error],
}),
],
})

0 comments on commit 91b2de2

Please sign in to comment.