[no-commonjs] add allowConditionalRequire option

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

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

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,

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