Skip to content

Commit

Permalink
[New] add no-import-module-exports rule: report import declarations…
Browse files Browse the repository at this point in the history
… with CommonJS exports

Fixes #760
  • Loading branch information
ttmarek authored and ljharb committed Apr 14, 2017
1 parent fe51583 commit 1b407b9
Show file tree
Hide file tree
Showing 8 changed files with 251 additions and 1 deletion.
6 changes: 5 additions & 1 deletion CHANGELOG.md
Expand Up @@ -10,6 +10,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- [`no-commonjs`]: Also detect require calls with expressionless template literals: ``` require(`x`) ``` ([#1958], thanks [@FloEdelmann])
- [`no-internal-modules`]: Add `forbid` option ([#1846], thanks [@guillaumewuip])
- add [`no-relative-packages`] ([#1860], [#966], thanks [@tapayne88] [@panrafal])
- add [`no-import-module-exports`] rule: report import declarations with CommonJS exports ([#804], thanks [@kentcdodds] and [@ttmarek])

### Fixed
- [`export`]/TypeScript: properly detect export specifiers as children of a TS module block ([#1889], thanks [@andreubotella])
Expand Down Expand Up @@ -732,6 +733,7 @@ for info on changes for earlier releases.
[`no-duplicates`]: ./docs/rules/no-duplicates.md
[`no-dynamic-require`]: ./docs/rules/no-dynamic-require.md
[`no-extraneous-dependencies`]: ./docs/rules/no-extraneous-dependencies.md
[`no-import-module-exports`]: ./docs/rules/no-import-module-exports.md
[`no-internal-modules`]: ./docs/rules/no-internal-modules.md
[`no-mutable-exports`]: ./docs/rules/no-mutable-exports.md
[`no-named-as-default-member`]: ./docs/rules/no-named-as-default-member.md
Expand Down Expand Up @@ -908,6 +910,7 @@ for info on changes for earlier releases.
[#871]: https://github.com/benmosher/eslint-plugin-import/pull/871
[#858]: https://github.com/benmosher/eslint-plugin-import/pull/858
[#843]: https://github.com/benmosher/eslint-plugin-import/pull/843
[#804]: https://github.com/benmosher/eslint-plugin-import/pull/804
[#797]: https://github.com/benmosher/eslint-plugin-import/pull/797
[#794]: https://github.com/benmosher/eslint-plugin-import/pull/794
[#744]: https://github.com/benmosher/eslint-plugin-import/pull/744
Expand Down Expand Up @@ -1330,4 +1333,5 @@ for info on changes for earlier releases.
[@bicstone]: https://github.com/bicstone
[@guillaumewuip]: https://github.com/guillaumewuip
[@tapayne88]: https://github.com/tapayne88
[@panrafal]: https://github.com/panrafal
[@panrafal]: https://github.com/panrafal
[@ttmarek]: https://github.com/ttmarek
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -67,11 +67,13 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a
* Report CommonJS `require` calls and `module.exports` or `exports.*`. ([`no-commonjs`])
* Report AMD `require` and `define` calls. ([`no-amd`])
* No Node.js builtin modules. ([`no-nodejs-modules`])
* Forbid imports with CommonJS exports ([`no-import-module-exports`])

[`unambiguous`]: ./docs/rules/unambiguous.md
[`no-commonjs`]: ./docs/rules/no-commonjs.md
[`no-amd`]: ./docs/rules/no-amd.md
[`no-nodejs-modules`]: ./docs/rules/no-nodejs-modules.md
[`no-import-module-exports`]: ./docs/rules/no-import-module-exports.md


### Style guide
Expand Down
74 changes: 74 additions & 0 deletions docs/rules/no-import-module-exports.md
@@ -0,0 +1,74 @@
# no-import-module-exports

Reports the use of import declarations with CommonJS exports in any module
except for the [main module](https://docs.npmjs.com/files/package.json#main).

If you have multiple entry points or are using `js:next` this rule includes an
`exceptions` option which you can use to exclude those files from the rule.

## Options

#### `exceptions`
- An array of globs. The rule will be omitted from any file that matches a glob
in the options array. For example, the following setting will omit the rule
in the `some-file.js` file.

```json
"import/no-import-module-exports": ["error", {
"exceptions": ["**/*/some-file.js"]
}]
```

## Rule Details

### Fail

```js
import { stuff } from 'starwars'
module.exports = thing

import * as allThings from 'starwars'
exports.bar = thing

import thing from 'other-thing'
exports.foo = bar

import thing from 'starwars'
const baz = module.exports = thing
console.log(baz)
```

### Pass
Given the following package.json:

```json
{
"main": "lib/index.js",
}
```

```js
import thing from 'other-thing'
export default thing

const thing = require('thing')
module.exports = thing

const thing = require('thing')
exports.foo = bar

import thing from 'otherthing'
console.log(thing.module.exports)

// in lib/index.js
import foo from 'path';
module.exports = foo;

// in some-file.js
// eslint import/no-import-module-exports: ["error", {"exceptions": ["**/*/some-file.js"]}]
import foo from 'path';
module.exports = foo;
```

### Further Reading
- [webpack issue #4039](https://github.com/webpack/webpack/issues/4039)
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -109,6 +109,7 @@
"is-core-module": "^1.0.2",
"minimatch": "^3.0.4",
"object.values": "^1.1.1",
"pkg-up": "^1.0.0",
"read-pkg-up": "^2.0.0",
"resolve": "^1.17.0",
"tsconfig-paths": "^3.9.0"
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Expand Up @@ -40,6 +40,7 @@ export const rules = {
'no-unassigned-import': require('./rules/no-unassigned-import'),
'no-useless-path-segments': require('./rules/no-useless-path-segments'),
'dynamic-import-chunkname': require('./rules/dynamic-import-chunkname'),
'no-import-module-exports': require('./rules/no-import-module-exports'),

// export
'exports-last': require('./rules/exports-last'),
Expand Down
66 changes: 66 additions & 0 deletions src/rules/no-import-module-exports.js
@@ -0,0 +1,66 @@
import minimatch from 'minimatch';
import path from 'path';
import pkgUp from 'pkg-up';

function getEntryPoint(context) {
const pkgPath = pkgUp.sync(context.getFilename());
return require.resolve(path.dirname(pkgPath));
}

module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Disallow import statements with module.exports',
category: 'Best Practices',
recommended: true,
},
fixable: 'code',
schema: [
{
'type': 'object',
'properties': {
'exceptions': { 'type': 'array' },
},
'additionalProperties': false,
},
],
},
create(context) {
const importDeclarations = [];
const entryPoint = getEntryPoint(context);
const options = context.options[0] || {};
let alreadyReported = false;

function report(node) {
const fileName = context.getFilename();
const isEntryPoint = entryPoint === fileName;
const isIdentifier = node.object.type === 'Identifier';
const hasKeywords = (/^(module|exports)$/).test(node.object.name);
const isException = options.exceptions &&
options.exceptions.some(glob => minimatch(fileName, glob));

if (isIdentifier && hasKeywords && !isEntryPoint && !isException) {
importDeclarations.forEach(importDeclaration => {
context.report({
node: importDeclaration,
message: `Cannot use import declarations in modules that export using ` +
`CommonJS (module.exports = 'foo' or exports.bar = 'hi')`,
});
});
alreadyReported = true;
}
}

return {
ImportDeclaration(node) {
importDeclarations.push(node);
},
MemberExpression(node) {
if (!alreadyReported) {
report(node);
}
},
};
},
};
1 change: 1 addition & 0 deletions tests/files/package.json
@@ -1,5 +1,6 @@
{
"dummy": true,
"main": "src-root/src-bar.js",
"devDependencies": {
"glob": "1.0.0",
"eslint": "2.x"
Expand Down
101 changes: 101 additions & 0 deletions tests/src/rules/no-import-module-exports.js
@@ -0,0 +1,101 @@
import path from 'path';
import { RuleTester } from 'eslint';

import { test } from '../utils';

const ruleTester = new RuleTester({
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
});
const rule = require('rules/no-import-module-exports');

const error = {
message: `Cannot use import declarations in modules that export using CommonJS ` +
`(module.exports = 'foo' or exports.bar = 'hi')`,
type: 'ImportDeclaration',
};

ruleTester.run('no-import-module-exports', rule, {
valid: [
test({
code: `
const thing = require('thing')
module.exports = thing
`,
}),
test({
code: `
import thing from 'otherthing'
console.log(thing.module.exports)
`,
}),
test({
code: `
import thing from 'other-thing'
export default thing
`,
}),
test({
code: `
const thing = require('thing')
exports.foo = bar
`,
}),
test({
code: `
import foo from 'path';
module.exports = foo;
`,
// When the file matches the entry point defined in package.json
// See tests/files/package.json
filename: path.join(process.cwd(), 'tests/files/src-root/src-bar.js'),
}),
test({
code: `
import foo from 'path';
module.exports = foo;
`,
filename: path.join(process.cwd(), 'tests/files/some/other/entry-point.js'),
options: [{ exceptions: ['**/*/other/entry-point.js'] }],
}),
],
invalid: [
test({
code: `
import { stuff } from 'starwars'
module.exports = thing
`,
errors: [error],
}),
test({
code: `
import thing from 'starwars'
const baz = module.exports = thing
console.log(baz)
`,
errors: [error],
}),
test({
code: `
import * as allThings from 'starwars'
exports.bar = thing
`,
errors: [error],
}),
test({
code: `
import thing from 'other-thing'
exports.foo = bar
`,
errors: [error],
}),
test({
code: `
import foo from 'path';
module.exports = foo;
`,
filename: path.join(process.cwd(), 'tests/files/some/other/entry-point.js'),
options: [{ exceptions: ['**/*/other/file.js'] }],
errors: [error],
}),
],
});

0 comments on commit 1b407b9

Please sign in to comment.