Register binding for newly created vars for commonjs transforms

[The-x-Theorist]

Q	A
Fixed Issues?	Fixes #13665 !-- remove the (`) quotes and write "Fixes" before the number to link the issues -->
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

Newly created vars will get registered as soon as path.unshiftContainer() happens.

[babel-bot]

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

[overlookmotel]

Nice one! Thanks loads for doing this.

I guess, strictly speaking, should also make sure that old bindings are also removed. e.g.:

// Input
import React from "react";

// Output
var _react = require("react");

So, as well as the new _react binding being created, the old React binding should be destroyed.

Also, vars which are used in CommonJS are renamed:

// Input
let module, exports, require, __dirname, __filename;

// Output
let _module, _exports, _require, _dirname, _filename;

Do bindings for these var name changes get dealt with too? (maybe they do automatically as they're just variable renaming not creation of new vars, I'm not sure)

I don't know how difficult it would be to handle the above. Certainly your PR as it is already is a big improvement on before.

[The-x-Theorist]

@overlookmotel Earlier there were no bindings registered for the newly created vars as of this, I think that there is no way we can remove the old bindings which weren't present actually.

[overlookmotel]

Ah sorry I wasn't clear. The React variable exists in the original source code, and so would have a binding (that's what I mean by the "old" binding). After the plugin has run, that var no longer exists, so should not have a binding any more.

Your PR creates a new binding for the new variable _react.

But I think the old binding for the old variable React will still exist, unless it's manually deleted.

Does that make more sense?

[The-x-Theorist]

@overlookmotel Now I think I understand, the old bindings should be deleted which were present before the plugin ran, like for React variable, Right?

[The-x-Theorist]

@overlookmotel Can you suggest a way with help of which oldBindings can be deleted?

[overlookmotel]

Yes, that's exactly what I meant.

I'm afraid no, I can't offer any suggestions. I'm not actually very familiar with the code in Babel which deals with scopes/bindings. Maybe one of the maintainers can offer some guidance.

Or it may be that it's fine to leave it as it is. The biggest fault was that the new bindings weren't registered (which you've solved). It's of lesser importance that the old bindings are removed.

[nicolo-ribaudo]

We should definitely remove the old bindings, but it can be done in a separate PR!

[nicolo-ribaudo]

Note for reviewers: this code is the same as

babel/packages/babel-core/src/transformation/file/file.ts

Lines 217 to 223 in bee3e35

	this.path.unshiftContainer("body", nodes);
	// TODO: NodePath#unshiftContainer should automatically register new
	// bindings.
	this.path.get("body").forEach(path => {
	if (nodes.indexOf(path.node) === -1) return;
	if (path.isVariableDeclaration()) this.scope.registerDeclaration(path);
	});

[nicolo-ribaudo]

Oh, it looks like I already have a PR for the "remove old bindings" part: #10640