Add allow-require option to no-commonjs rule

[futpib]

This fixes #548

[coveralls]

Coverage increased (+0.006%) to 95.969% when pulling d8dfc3b on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[ljharb]

Let's use an object schema instead, so it's extensible - like { allowRequire: true, allowPrimitiveModules: true }.

We should move away from string schemas (but continue to support the existing ones, ofc)

[ljharb]

When you have this rule enabled, wouldn't you use import() for conditionals?

[futpib]

Well, you could. I guess it's for compatibility only then.

[futpib]

Actually, I just looked though some babel-plugin-dynamic-import-node tests, the new dynamic import is async and thus is not a drop-in replacement for require. So I guess adding a hint like this would suffice:

This is useful for conditional requires. If you don't rely on synchronous module loading, check out dynamic import.

[ljharb]

True enough; your suggestion is a good addition.

[benmosher]

This was my knee jerk reaction as well, but I think the sync argument tracks.

[ljharb]

Can we add a schema to this rule so eslint can validate it?

[coveralls]

Coverage increased (+0.01%) to 95.976% when pulling 11d6d51 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[ljharb]

Thanks, this looks great!

[ljharb]

i think this first anyOf item can just be schemaString by itself?

[futpib]

I don't think so, options are always an array of things.

[coveralls]

Coverage increased (+0.04%) to 96.008% when pulling 759efd4 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[coveralls]

Coverage increased (+0.04%) to 96.008% when pulling 759efd4 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[coveralls]

Coverage increased (+0.04%) to 96.008% when pulling 759efd4 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[coveralls]

Coverage increased (+0.04%) to 96.008% when pulling 759efd4 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[coveralls]

Coverage increased (+0.04%) to 96.008% when pulling 759efd4 on futpib:no-commonjs-allow-require into 8101d39 on benmosher:master.

[coveralls]

Coverage increased (+0.01%) to 96.47% when pulling c438d3e on futpib:no-commonjs-allow-require into 48d0a8a on benmosher:master.

[futpib]

I'm so waiting for this to release. Is there anything else to do for me?

[ljharb]

@futpib i'd like @benmosher or @jfmengels to review it prior to merging.

[benmosher]

Hi @futpib, sorry it has been literally forever since you submitted this and I'm finally getting back to it. 😅

I just upgraded this to semver-major because it changes the options on this rule. If you'd like to get it merged into the next 2.x release, can you make the options match the existing rule?

i.e. 'allow-require' instead of allowRequire: true. This is not my first choice; I like the flags-based options you've implemented better, but again, they are a breaking change to the existing rule.

[benmosher]

see my note in the main PR conversation thread, but tl;dr: extend the existing allow-primitive-modules enum to be set as allow-requires.

I think it's okay--or even ideal--if they are mutually exclusive, as IIRC enabling both would turn off the rule and is therefore a nonsense config state.

[benmosher]

nvm, I totally read this wrong and you are fine 😅

I misread the new tests as changes, but you left the existing tests in place so obviously it's semver-minor, my bad!

[futpib]

@benmosher still got semver-major label though