Avoid duplicate side-effects-only require() calls

[nicolo-ribaudo]

Fixes babel/babel#12583 (comment)

[nicolo-ribaudo]

Here we have two require calls because we first inject

import { foo as _foo1 } from "./polyfill/foo";

_foo1;

then the CJS transform generates

var _foo2 = require("./polyfill/foo");

_foo2.foo;

Then the _foo1 binding is no more available, so the polyfill provider injects a new require().

[ljharb]

isn't that the bug this PR is trying to fix?

[nicolo-ribaudo]

It's only fixable (with a not too much compilcated logic) for require calls whose result isn't used (the first test).

[ljharb]

Since the CJS transform is trying to replicate live bindings, shouldn't it find all instances of _foo1 and change them to _foo2.foo, which then shouldn't require an additional require to be added by the polyfill provider?

[nicolo-ribaudo]

It does, and it generates this code:

var _foo2 = require("./polyfill/foo");

_foo2.foo;

Then another plugin (the one that does path.pushContainer in the test) injects a new reference to the foo "built-in":

var _foo2 = require("./polyfill/foo");

_foo2.foo;
foo;

At this point, the polyfill plugin needs to polyfill the second foo. Since require calls are not easily statically analyzable, we inject a new require rather than traversing the AST to find a variable whose value is the result of require("./polyfill/foo").

In practice this doesn't have a big effect because when using bundlers, you don't transpile to CJS with Babel: this mostly affects Node.js users, and the second require call is a noop because of the require cache.

[ljharb]

require calls with a string literal are just as statically analyzeable as static ESM tho.

[ljharb]

Also, you definitely do transpile to CJS using babel with some bundlers. Only webpack has you transpile to ES5 but leave in the otherwise-invalid import/export statements, afaik.

[nicolo-ribaudo]

The main problem is that we would have to visit at least all the nodes in the top-level every time we inject a require() call for a new polyfill that misses the cache (either because it's a new polyfill, or because it was injected in ESM and then compiled to CJS). We currently don't need to do this (neither for CJS nor for ESM), because we cache what we inject. This cache is invalidated when the sourceType changes.

Anyway, this is about a few bytes that are easily gzipped (because they are exactly the same call), and that usually aren't injected (because the modules transforms runs on Program:exit, that usually is the last thing).

I'm merging this PR as-is because it solves the easy case (global polyfills), but I'll keep in mind that the pure polyfill case isn't ideal yet.

(Also Rollup and Parcel don't use Babel to compile ESM, afaik Browserify is the only popular bundler that works only with require())