feat: helper-module-imports adds specifiers to existing imports

[conartist6]

Q	A
Fixed Issues?	Fixes #14483
Patch: Bug Fix?	No
Major: Breaking Change?	???
Minor: New Feature?	???
Tests Added + Pass?	Yes
Documentation PR Link	none
Any Dependency Changes?	no
License	MIT

This allows @babel/helper-module-imports to behave more like users likely expect it to by combining multiple import statements into a single import.

For example writing this in your plugin:

  addSideEffect("source");
  addNamespace("source", { nameHint: "ns" });
  addDefault("source");
  addNamed("read", "source");
```js
Now outputs:
```js
import * as _ns, _default, { read as _read } from "source";

Furthermore existing imports are reused when possible, e.g. if you have

import { read } from "source";

and you write addNamed("read", "source") instead of binding and returning a new _read local name the existing read name will be returned.

There was some existing functionality here -- imports with the same source were being merged later on, but this was too late to avoid ending up with multiple local names for the same imported name.

This PR also changes the insertion behavior for imports whose interop implementation consists of an import declaration and and a var declaration. The var declaration will now always be inserted below any other imports which are present. This solved a specific problem in the implementation: when an existing import declaration is reused it is not reordered. Thus statements might contain only var declarations relevant to some existing import. In that case the unchanged algorithm inserting above causes output like:

var _source = babelHelpers.interopRequireWildcard(_source$es6Default);
import _source$es6Default from "source";

I'm not sure how to categorize this in terms of semver. There is no documentation of how this works, meaning the implementation is essentially the spec and any change at all should probably be considered breaking.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51860/

[conartist6]

I still don't really understand the purpose of prefixing everything with _. It seems like a pattern designed to make namespace conflicts less common between identifiers the user defined and those inserted during the transpilation process. I imagine it being possible (and hopefully common) to use this helper when writing codemods, so it seems to me that if a nameHint is present there is no reason to prefix the hinted name with _ unless there is an actual name conflict.

[JLHwung]

combining multiple import statements into a single import.

The optimization holds when the imported library has no side-effect.

import { a1 } from "./module-a"
import b from "./module-b"
import { a2 } from "./module-a"

In this example, the module-b could introduce side-effects which interferes with module-a's behaviour. So we cannot merge the a1 and a2 named imports. I think the current add* behaviour is okay, we could introduce new helpers that counts for the duplicate imports.

[conartist6]

@JLHwung I don't think your example is good.
If this is safe:

import { a1 } from "./module-a"
import b from "./module-b"
import { a2 } from "./module-a"

then this is also safe:

import { a1, a2 } from "./module-a"
import b from "./module-b"

What you seem to be suggesting is that it would be unsafe NOT to create a potentially breaking change in the import order!!

For example you are suggesting that when the code is:

import b from "./module-b"
import { a2 } from "./module-a"

You expect addSideEffect('module-a') to change the code to:

import "./module-a"
import b from "./module-b"
import { a2 } from "./module-a"

I agree that it is possible that people are relying on this behavior, but since this package does not follow semantic versioning since it has no API documentation I'd argue that it only matters if it breaks babel. It's very, very bad practice to use an import for its side effect but also for exports. I assume babel does not do this. Does it?

[conartist6]

Another example is even more useful to me:

import 'module_a';
import 'module_b';

Since these imports are used for their side effects, I would expect that these statements never change order, which is what these changes ensure.

[conartist6]

OK, and I've pushed a second commit now that finally does what I came here to do: ensure that it's possible to use @babel/helper-module-imports in codemods. The plainImports option makes the builder create

import { name } from 'source';

instead of

import { name as _name } from 'source';

[conartist6]

I have to think carefully about how to make test coverage for the plainUids option. Test coverage isn't as urgently needed since we don't expect babel itself to use this option. I can't in good conscience copy all the test cases again since there's already so many of them.

[conartist6]

I've purposefully separated these changes into three commits. One to reuse existing identifiers, one to allow scope to generate unique ids without _ prefixes, and one to add the plainUids option to helper-imports so that it can doesn't have to turn every import it touches into name as _name. Taken together these changes allow this plugin to be used when writing codemods!

[conartist6]

Ah, I didn't realize up til now that you couldn't legally have an importNamespaceSpecifier along with any other kind of import specifier.

The way I would handle this as a human is to always give the namespace import specifier priority, then make a variable that holds a particular identifier as a property of the namespace. That is to say if I'm given

import * as ns from 'source';

then when addNamed('name', 'source') is called we should end up with:

import * as ns from 'source';
var _name = ns.name;

Of course we'd want to handle this the other way around as well where the named specifiers are already imported but the API is used to create a namespace import.

[conartist6]

I'm catching some of these cases in tests for other modules. They really need to be tested in the test cases for helper- imports though so I think I'll make a new test file that isn't as rigidly structured as the existing one.

[conartist6]

OK, import namespace specifiers are fixed -- the helper uses the namespace specifier and determines statements which reference it.

For codemods I'll also want to ensure that it's possible to configure the helper to generate some higher level syntax like destructuring const assignment.

[conartist6]

For codemods we don't have to worry about interop since codemods are source -> source, where interop is added when a source is compiled. Thus destructuring const assignment only matters when some combination of a namespace import and some other import occur, but then it does matter.

It should be pretty doable to create a de-facto destructuring assignments section immediately below the imports. The hard question is: when we are searching for an existing destructuring assignment expression to combine with, how do we know if the destructuring assignment we find might be too late in the program?

For example if we start with:

import * as ns from 'source';
const { foo } = ns;

then addNamed('bar', 'source');, we should end up with:

import * as ns from 'source';
const { foo, bar } = ns;

So far so good. But what if that destructuring assignment hadn't been at the top? What if a codemod had inserted something above it for example?

import * as ns from 'source';

// codemod inserts this
console.log(bar);

const { foo } = ns;
console.log(foo);

Now if we attempt to merge the assignments we'd get the incorrect code:

import * as ns from 'source';

console.log(bar);

const { foo, bar } = ns;
console.log(foo);

I see two possible options for resolving this.

1. Be as smart as possible. Track references and try to allow any combination of specifiers that might be safe.
2. Be as dumb as possible. Reserve space immediately below the imports for consecutive expressions of the form const {...} = identifier. Only merge specifiers for declarations in this space.

I'm leaning towards option two because it's faster, easier to write, and in most circumstances does the right thing. There's very low cost to false negative result as to whether two destructuring assignments from a namespace could have been merged.

[conartist6]

Thinking some more about mixing side effect imports with named or default imports. Perhaps that could/should be forbidden? It seems to me like if it occurs it's very likely a programming error in my plugin or codemod that I would want to know about.

[conartist6]

Alright I'm done messing around with this for now. I have what I need. All the other stuff I've mentioned here can be formally proposed in later PRs.

[conartist6]

Just kidding I made one more change, I decided that it was sufficiently safe to just add a second argument to scope.generateUid. Now you can make a call like:

scope.generateUid(name, (i) => (i > 1 ? `${name}_${i}` : name));

This is easily the best way of dealing with this I think as it gives the caller the freedom to use any disambiguation scheme they prefer, e.g. they could disambiguate as name -> name_ -> name__ ... with the trivial (i, name) => name + '_'.repeat(i - 1).

[conartist6]

Closing this as #16349 was merged instead