From a45661bbb37e332511bd40854b2bf0377a608787 Mon Sep 17 00:00:00 2001 From: Thomas Marek Date: Fri, 14 Apr 2017 17:18:23 -0400 Subject: [PATCH] [New] add `no-import-module-exports` rule: report import declarations with CommonJS exports Fixes #760 --- CHANGELOG.md | 6 +- README.md | 2 + docs/rules/no-import-module-exports.md | 74 ++++++++++++++ package.json | 1 + src/index.js | 1 + src/rules/no-import-module-exports.js | 66 +++++++++++++ tests/src/rules/no-import-module-exports.js | 101 ++++++++++++++++++++ 7 files changed, 250 insertions(+), 1 deletion(-) create mode 100644 docs/rules/no-import-module-exports.md create mode 100644 src/rules/no-import-module-exports.js create mode 100644 tests/src/rules/no-import-module-exports.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 486c0248a..ac420f24c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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]) @@ -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 @@ -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 @@ -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 \ No newline at end of file +[@panrafal]: https://github.com/panrafal +[@ttmarek]: https://github.com/ttmarek \ No newline at end of file diff --git a/README.md b/README.md index e08e72ffa..7945546a6 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/docs/rules/no-import-module-exports.md b/docs/rules/no-import-module-exports.md new file mode 100644 index 000000000..8131fd5f7 --- /dev/null +++ b/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) diff --git a/package.json b/package.json index 654e7fe1b..70afd1172 100644 --- a/package.json +++ b/package.json @@ -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" diff --git a/src/index.js b/src/index.js index 17002337e..7fa3710d6 100644 --- a/src/index.js +++ b/src/index.js @@ -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'), diff --git a/src/rules/no-import-module-exports.js b/src/rules/no-import-module-exports.js new file mode 100644 index 000000000..7ac56da39 --- /dev/null +++ b/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); + } + }, + }; + }, +}; diff --git a/tests/src/rules/no-import-module-exports.js b/tests/src/rules/no-import-module-exports.js new file mode 100644 index 000000000..bd18bf477 --- /dev/null +++ b/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/index.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], + }), + ], +});