Nit: we can hoist this check outside of the for loop:

const needsRename = path.node.kind !== "var" && path.parentPath.isBlockStatement()

Actually, I think we can just edit the Scope visitor above to check kind === "let" || kind === "const".

Okay, I will change that.

This behavior should not change.

The TS warning here is correct; I think this breaks, for example, with const { x } = {}. You probably need to do

if (needsRename) {
  for (const name of Object.keys(declar.getBindingIdentifiers()) {
    declar.scope.rename(name);
  }
}

Okay, I will do that.

Also add a test for this!

Okay, I will add the test

I think that it's working now.

Why const { x } = { x: 1 } is also transformed? I would expect _x is registered in the same scope of the local x declaration:

if (true) {
  const { x: _x } = { x: 1 };
  console.log(_x);
}

Hoisted vars are renamed, So I think that's the reason for this.

What should we change in this?

I have been thinking about this, and the proper fix probably is in the systemjs plugin itself.

We should not hoist all variables (var and let/const), but only var and top-level let/const. We can do this by:

1. In the big loop over the program body (in the Program.exit visitor), we convert all the top-level let/const bindings to var. This is safe, they are not in nested blocks.
2. When calling hoistVariables, we should remove the third parameter (null) so that it only hoists var and not also let/const.

For (1), be careful because export let x = 1; and export { x }; let x = 1; are handled in two different code paths and both should still work.

If third parameter is removed from the hoistVariable it removes all the _export calls from the outputs, and doesn't transform the input?

Yeah we will still need to hoist these export bindings and set up the metadata from which the _export calls are built. I suggest we copy the code from the hoistVariable to the systemjs transform, and implement the custom hoisting logic mentioned in #14057 (comment). Or we can add a new option in the helper, though I lean to the first option since the hoisting logic here is not reused by other plugins.

I'm surprised that the current helper doesn't work, because we only need _export for things that become var in step 1 🤔
Do you mind pushing the current (non working) code you have?

I don't have a working branch. We can infer the behaviour of hoistVariable helper from the implementation:

The third variable kind controls two behaviours: 1) Whether we should check a BlockParent to see if it contains to-be-hoisted variables, and 2) whether we should hoist the let-kind variable declarations.

In systemjs transform, what we want to achieve is to hoist var variables within block parent (so kind has to be "let"), but leave let-kind declarations within block parent untouched.

• If we set kind to "var", only var-kind declarations will be hoisted, the top level let-kinds are not.
• If we set kind to null, all declarations will be hoisted unless nested within FunctionParent. But we want to preserve let within block parent
• If we set kind to "let", only let-kind declarations will be hoisted

Therefore we still need to extend the hoistVariable helper behaviour.

@The-x-Theorist I hope you don't mind, I pushed two commits to your branch.

14e9cfd (#14057) is how I was thinking to fix the problem, as described in #14057 (comment):

1. It changes all the top-level let/const to var
2. It only hoists vars

The updated tests look good, but it probably needs a few new tests to make sure that both const x = 1; export { x } and export const x = 1; work (unless we already have these tests).

A top level var declaration can be nested within for statement:

for (var x = 1;false;);
export { x }

or it could be within a do expression.

They already have var, there is no need to manually change them.

The hoistVariable visitor should be merged with environmentVisitor, too.

Lets do it in another PR since it's separate from this bug 👍