Eslint-plugin-import: Allow require() but forbid module.exports

[Wilfred]

I'm porting a codebase to ES6. In some cases we explicitly want to use require(), because import must be at the top of the file.

For example, we sometimes write:

if (process.env.NODE_ENV == "production") {
  require('../foo.css');
}

or in tests we sometimes want to only require things at certain points. We also conditionally require polyfills.

Could we add a configuration option to allow require() inside no-commonjs? Alternatively, would it make sense to split the rule into no-commonjs-require and no-commonjs-exports?

[benmosher]

Are you doing so frequently enough that

if (process.env.NODE_ENV == "production") {
  require('../foo.css'); // eslint-disable-line import/no-commonjs
}

would be burdensome?

[Wilfred]

Yep, I think so. Our current workaround is (ab)using no-restricted-globals to simply blacklist module.

[benmosher]

what if require were only allowed when not at top-level scope, behind a flag?

i.e. you can set allowDynamicRequire: true and it would allow your example, but top-level requires would still be reported?

// allowDynamicRequire: true
const fs = require('fs') // still reported
if (process.env.NODE_ENV == "production") {
  require('../foo.css'); // fine
}

[Wilfred]

That would cover most cases, but in some cases we explicitly use require to ensure ordering. Since import is hoisted, we use require when we need to control when the code is loaded.

if (!window.Promise) {
  require('./promise-shim');
}

// Load Foo *after* we have shim in place.
const Foo = require('./Foo').default;

[jfmengels]

@benmosher I don't mind having this, but this should then probably be included in v2. At least if we want to split the rule into two and then remove the original one.

Either that or have an option that allows either require, which might be a bit simpler.

[benmosher]

Yeah, I'm leaning towards flags on this. So probably no breaking changes, I still like the default rule just reporting on all CJS usage, regardless of whatever modifications/new rules.

[jfmengels]

Agreed on the flags and defaults.
Let's just add a flag to allow require() for now, and wait for the module.exports until someone requests it with good reason. Would that work for you @Wilfred?

[Wilfred]

Yep, that would work for me.

[benmosher]

actually I'm thinking the setting should be

require: "allow" | "deny" (default) | "dynamic"

@Wilfred you could use "allow" to support both your cases, but I also like having a mode that allows dynamic requires (I would probably use that at work actually).

[benmosher]

also not super-attached to the word dynamic, would be up for bikeshedding a more obvious name for that mode if anyone has ideas.

[jfmengels]

@benmosher What would dynamic be? A require with an expression as the argument, or one in a conditional statement/other construct?

[benmosher]

one in a conditional statement/other construct

i.e.

const someVariedImport = (someCondition) ? require('one') : require('other');

[benmosher]

Expression as the argument would be valid qualify as dynamic too, I suppose. I've just never needed to do that. I'm not sure that this rule would even trigger on that ATM anyway, I think it looks for a single string literal argument.

[jfmengels]

dynamic is used as the term for no-dynamic-require, maybe that's not a good name too.

Where I see requires the most in the codebases, is with hot-reloading, so

import foo from './foo';
if (module.hot) {
  const foo = require('./foo');
  //...
}

I'm not sure that this rule would even trigger on that ATM anyway

Yes, that looks like what it is doing.

[benmosher]

@[potential implementer]: don't feel like you have to implement dynamic mode, a PR implementing only allow / deny would be great too.