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 rule 'no-self-import' #727

Merged
merged 1 commit into from
Jan 4, 2018
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Yanked due to critical issue in eslint-module-utils with cache key resulting fro
- [`no-anonymous-default-export`] rule: report anonymous default exports ([#712], thanks [@duncanbeevers]).
- Add new value to [`order`]'s `newlines-between` option to allow newlines inside import groups ([#627], [#628], thanks [@giodamelio])
- Add `count` option to the [`newline-after-import`] rule to allow configuration of number of newlines expected ([#742], thanks [@ntdb])
- Add [`no-self-import`] rule: forbids a module from importing itself. ([#727], [#449], [#447], thanks [@giodamelio]).

### Changed
- [`no-extraneous-dependencies`]: use `read-pkg-up` to simplify finding + loading `package.json` ([#680], thanks [@wtgtybhertgeghgtwtg])
Expand Down Expand Up @@ -433,6 +434,7 @@ for info on changes for earlier releases.
[`no-anonymous-default-export`]: ./docs/rules/no-anonymous-default-export.md
[`exports-last`]: ./docs/rules/exports-last.md
[`group-exports`]: ./docs/rules/group-exports.md
[`no-self-import`]: ./docs/rules/no-self-import.md

[`memo-parser`]: ./memo-parser/README.md

Expand All @@ -444,6 +446,7 @@ for info on changes for earlier releases.
[#744]: https://github.com/benmosher/eslint-plugin-import/pull/744
[#742]: https://github.com/benmosher/eslint-plugin-import/pull/742
[#737]: https://github.com/benmosher/eslint-plugin-import/pull/737
[#727]: https://github.com/benmosher/eslint-plugin-import/pull/727
[#721]: https://github.com/benmosher/eslint-plugin-import/pull/721
[#712]: https://github.com/benmosher/eslint-plugin-import/pull/712
[#696]: https://github.com/benmosher/eslint-plugin-import/pull/696
Expand All @@ -468,6 +471,7 @@ for info on changes for earlier releases.
[#489]: https://github.com/benmosher/eslint-plugin-import/pull/489
[#485]: https://github.com/benmosher/eslint-plugin-import/pull/485
[#461]: https://github.com/benmosher/eslint-plugin-import/pull/461
[#449]: https://github.com/benmosher/eslint-plugin-import/pull/449
[#444]: https://github.com/benmosher/eslint-plugin-import/pull/444
[#428]: https://github.com/benmosher/eslint-plugin-import/pull/428
[#395]: https://github.com/benmosher/eslint-plugin-import/pull/395
Expand Down Expand Up @@ -533,6 +537,7 @@ for info on changes for earlier releases.
[#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
[#447]: https://github.com/benmosher/eslint-plugin-import/issues/447
[#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
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Forbid `require()` calls with expressions ([`no-dynamic-require`])
* Prevent importing the submodules of other modules ([`no-internal-modules`])
* Forbid Webpack loader syntax in imports ([`no-webpack-loader-syntax`])
* Forbid a module from importing itself ([`no-self-import`])

[`no-unresolved`]: ./docs/rules/no-unresolved.md
[`named`]: ./docs/rules/named.md
Expand All @@ -33,6 +34,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
[`no-dynamic-require`]: ./docs/rules/no-dynamic-require.md
[`no-internal-modules`]: ./docs/rules/no-internal-modules.md
[`no-webpack-loader-syntax`]: ./docs/rules/no-webpack-loader-syntax.md
[`no-self-import`]: ./docs/rules/no-self-import.md

**Helpful warnings:**

Expand Down
30 changes: 30 additions & 0 deletions docs/rules/no-self-import.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Forbid a module from importing itself

Forbid a module from importing itself. This can sometimes happen during refactoring.

## Rule Details

### Fail

```js
// foo.js
import foo from './foo';

const foo = require('./foo');
```

```js
// index.js
import index from '.';

const index = require('.');
```

### Pass

```js
// foo.js
import bar from './bar';

const bar = require('./bar');
```
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const rules = {
'no-internal-modules': require('./rules/no-internal-modules'),
'group-exports': require('./rules/group-exports'),

'no-self-import': require('./rules/no-self-import'),
'no-named-default': require('./rules/no-named-default'),
'no-named-as-default': require('./rules/no-named-as-default'),
'no-named-as-default-member': require('./rules/no-named-as-default-member'),
Expand Down
41 changes: 41 additions & 0 deletions src/rules/no-self-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* @fileOverview Forbids a module from importing itself
* @author Gio d'Amelio
*/

import resolve from 'eslint-module-utils/resolve'
import isStaticRequire from '../core/staticRequire'

function isImportingSelf(context, node, requireName) {
const filePath = context.getFilename()
Copy link
Member

Choose a reason for hiding this comment

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

what happens when the input is from stdin, and thus there's no filename?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! When that happens, filePath will equal '<text>. In some other plugin, we've handled it like: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/rules/filename-case.js#L71-L73

Copy link
Member

Choose a reason for hiding this comment

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

That seems totally reasonable - it might also be useful to create a generic way to tag rules as "does not apply when the file is from stdin"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have actually never used the --stdin option and don't know the uses for it 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't even know that was a thing you could do.

Copy link
Member

Choose a reason for hiding this comment

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

I use it all the time when providing reproduction steps for eslint bugs :-p

Copy link
Member

Choose a reason for hiding this comment

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

last I knew, Sublime uses --stdin, but with --stdin-filename (or something like that) to fake in the filename


// If the input is from stdin, this test can't fail
if (filePath !== '<text>' && filePath === resolve(requireName, context)) {
context.report({
node,
message: 'Module imports itself.',
})
}
}

module.exports = {
meta: {
doc: {
description: 'Forbid a module from importing itself',
recommended: true,
},
schema: [],
},
create: function (context) {
return {
ImportDeclaration(node) {
isImportingSelf(context, node, node.source.value)
},
CallExpression(node) {
if (isStaticRequire(node)) {
isImportingSelf(context, node, node.arguments[0].value)
}
},
}
},
}
1 change: 1 addition & 0 deletions tests/files/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// Used in `no-self-import` tests
1 change: 1 addition & 0 deletions tests/files/no-self-import-folder/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// Used in `no-self-import` tests
1 change: 1 addition & 0 deletions tests/files/no-self-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// Used in `no-self-import` tests
121 changes: 121 additions & 0 deletions tests/src/rules/no-self-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
import { test, testFilePath } from '../utils'

import { RuleTester } from 'eslint'

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

const error = {
ruleId: 'no-self-import',
message: 'Module imports itself.',
}

ruleTester.run('no-self-import', rule, {
valid: [
test({
code: 'import _ from "lodash"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import find from "lodash.find"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import foo from "./foo"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import foo from "../foo"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import foo from "foo"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import foo from "./"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'import foo from "@scope/foo"',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var _ = require("lodash")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var find = require("lodash.find")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var foo = require("./foo")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var foo = require("../foo")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var foo = require("foo")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var foo = require("./")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var foo = require("@scope/foo")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var bar = require("./bar/index")',
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var bar = require("./bar")',
filename: testFilePath('./bar/index.js'),
}),
test({
code: 'var bar = require("./bar")',
filename: '<text>',
}),
],
invalid: [
test({
code: 'import bar from "./no-self-import"',
errors: [error],
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var bar = require("./no-self-import")',
errors: [error],
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var bar = require("./no-self-import.js")',
errors: [error],
filename: testFilePath('./no-self-import.js'),
}),
test({
code: 'var bar = require(".")',
errors: [error],
filename: testFilePath('./index.js'),
}),
test({
code: 'var bar = require("./")',
errors: [error],
filename: testFilePath('./index.js'),
}),
test({
code: 'var bar = require("././././")',
errors: [error],
filename: testFilePath('./index.js'),
}),
test({
code: 'var bar = require("../no-self-import-folder")',
errors: [error],
filename: testFilePath('./no-self-import-folder/index.js'),
}),
],
})