[no-commonjs] does not catch conditional require

[Pessimistress]

The example here: https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-commonjs.md#allow-require

cannot be reported even if allowRequire: false, because of this line:

https://github.com/benmosher/eslint-plugin-import/blob/1edbbd03ec636469328ad59da922ed7edc8cd36d/src/rules/no-commonjs.js#L90

The context scope is block in this case.

[ljharb]

Looks like we need some failing test cases then, thanks.

[Pessimistress]

@ljharb I can open a PR to fix this. However these test cases confuse me:

https://github.com/benmosher/eslint-plugin-import/blob/1edbbd03ec636469328ad59da922ed7edc8cd36d/tests/src/rules/no-commonjs.js#L31-L32

Is this by design? Should I add a new option if I want to strictly forbid any usage of require?

[ljharb]

Yes, if you’d like to submit 2 PRs - one for this issue, and one to disallow all usage of require - please do.

[Pessimistress]

I could theoretically fix my issue without breaking the current test cases, though shouldn't these two scenarios be treated the same?

var a = c && require("b")

if (c) {
  require("b")
}

[ljharb]

Yes, in theory, but since they’re different scopes it’s not necessarily the same.