diff --git a/CHANGELOG.md b/CHANGELOG.md index c5562da2e..c7c96bcd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel - [`order`]: Adds support for pathGroups to allow ordering by defined patterns ([#795], [#1386], thanks [@Mairu]) - [`no-duplicates`]: Add `considerQueryString` option : allow duplicate imports with different query strings ([#1107], thanks [@pcorpet]). - [`order`]: Add support for alphabetical sorting of import paths within import groups ([#1360], [#1105], [#629], thanks [@duncanbeevers], [@stropho], [@luczsoma], [@randallreedjr]) +- [`no-commonjs`]: add `allowConditionalRequire` option ([#1439], thanks [@Pessimistress]) ### Fixed - [`default`]: make error message less confusing ([#1470], thanks [@golopot]) @@ -624,6 +625,7 @@ for info on changes for earlier releases. [#1493]: https://github.com/benmosher/eslint-plugin-import/pull/1493 [#1472]: https://github.com/benmosher/eslint-plugin-import/pull/1472 [#1470]: https://github.com/benmosher/eslint-plugin-import/pull/1470 +[#1439]: https://github.com/benmosher/eslint-plugin-import/pull/1439 [#1436]: https://github.com/benmosher/eslint-plugin-import/pull/1436 [#1435]: https://github.com/benmosher/eslint-plugin-import/pull/1435 [#1425]: https://github.com/benmosher/eslint-plugin-import/pull/1425 @@ -1026,3 +1028,4 @@ for info on changes for earlier releases. [@luczsoma]: https://github.com/luczsoma [@christophercurrie]: https://github.com/christophercurrie [@randallreedjr]: https://github.com/randallreedjr +[@Pessimistress]: https://github.com/Pessimistress diff --git a/docs/rules/no-commonjs.md b/docs/rules/no-commonjs.md index 4353886bf..7be4bb399 100644 --- a/docs/rules/no-commonjs.md +++ b/docs/rules/no-commonjs.md @@ -27,15 +27,30 @@ If `allowRequire` option is set to `true`, `require` calls are valid: ```js /*eslint no-commonjs: [2, { allowRequire: true }]*/ +var mod = require('./mod'); +``` + +but `module.exports` is reported as usual. + +### Allow conditional require + +By default, conditional requires are allowed: + +```js +var a = b && require("c") if (typeof window !== "undefined") { require('that-ugly-thing'); } + +var fs = null; +try { + fs = require("fs") +} catch (error) {} ``` -but `module.exports` is reported as usual. +If the `allowConditionalRequire` option is set to `false`, they will be reported. -This is useful for conditional requires. If you don't rely on synchronous module loading, check out [dynamic import](https://github.com/airbnb/babel-plugin-dynamic-import-node). ### Allow primitive modules diff --git a/src/rules/no-commonjs.js b/src/rules/no-commonjs.js index 261654bbf..456f030f4 100644 --- a/src/rules/no-commonjs.js +++ b/src/rules/no-commonjs.js @@ -25,6 +25,26 @@ function allowRequire(node, options) { return options.allowRequire } +function allowConditionalRequire(node, options) { + return options.allowConditionalRequire !== false +} + +function validateScope(scope) { + return scope.variableScope.type === 'module' +} + +// https://github.com/estree/estree/blob/master/es5.md +function isConditional(node) { + if ( + node.type === 'IfStatement' + || node.type === 'TryStatement' + || node.type === 'LogicalExpression' + || node.type === 'ConditionalExpression' + ) return true + if (node.parent) return isConditional(node.parent) + return false +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -35,6 +55,7 @@ const schemaObject = { properties: { allowPrimitiveModules: { 'type': 'boolean' }, allowRequire: { 'type': 'boolean' }, + allowConditionalRequire: { 'type': 'boolean' }, }, additionalProperties: false, } @@ -87,12 +108,7 @@ module.exports = { }, 'CallExpression': function (call) { - if (context.getScope().type !== 'module') return - if ( - call.parent.type !== 'ExpressionStatement' - && call.parent.type !== 'VariableDeclarator' - && call.parent.type !== 'AssignmentExpression' - ) return + if (!validateScope(context.getScope())) return if (call.callee.type !== 'Identifier') return if (call.callee.name !== 'require') return @@ -105,6 +121,8 @@ module.exports = { if (allowRequire(call, options)) return + if (allowConditionalRequire(call, options) && isConditional(call.parent)) return + // keeping it simple: all 1-string-arg `require` calls are reported context.report({ node: call.callee, diff --git a/tests/src/rules/no-commonjs.js b/tests/src/rules/no-commonjs.js index 8ca8fde50..1bcbc65ab 100644 --- a/tests/src/rules/no-commonjs.js +++ b/tests/src/rules/no-commonjs.js @@ -56,6 +56,13 @@ ruleTester.run('no-commonjs', require('rules/no-commonjs'), { { code: 'module.exports = function () {}', options: [{ allowPrimitiveModules: true }] }, { code: 'module.exports = "foo"', options: ['allow-primitive-modules'] }, { code: 'module.exports = "foo"', options: [{ allowPrimitiveModules: true }] }, + + { code: 'if (typeof window !== "undefined") require("x")', options: [{ allowRequire: true }] }, + { code: 'if (typeof window !== "undefined") require("x")', options: [{ allowRequire: false }] }, + { code: 'if (typeof window !== "undefined") { require("x") }', options: [{ allowRequire: true }] }, + { code: 'if (typeof window !== "undefined") { require("x") }', options: [{ allowRequire: false }] }, + + { code: 'try { require("x") } catch (error) {}' }, ], invalid: [ @@ -65,6 +72,19 @@ ruleTester.run('no-commonjs', require('rules/no-commonjs'), { { code: 'var x = require("x")', errors: [ { message: IMPORT_MESSAGE }] }, { code: 'x = require("x")', errors: [ { message: IMPORT_MESSAGE }] }, { code: 'require("x")', errors: [ { message: IMPORT_MESSAGE }] }, + + { code: 'if (typeof window !== "undefined") require("x")', + options: [{ allowConditionalRequire: false }], + errors: [ { message: IMPORT_MESSAGE }], + }, + { code: 'if (typeof window !== "undefined") { require("x") }', + options: [{ allowConditionalRequire: false }], + errors: [ { message: IMPORT_MESSAGE }], + }, + { code: 'try { require("x") } catch (error) {}', + options: [{ allowConditionalRequire: false }], + errors: [ { message: IMPORT_MESSAGE }], + }, ]), // exports