[systemjs] Fix nested let/const shadowing imported bindings (#14057)

The-x-Theorist · nicolo-ribaudo · web-flow · commit 410c9acf1b92 · 2022-03-17T12:41:06.000+01:00
Co-authored-by: Nicolò Ribaudo &lt;nicolo.ribaudo@gmail.com&gt;
diff --git a/packages/babel-plugin-transform-modules-systemjs/src/index.ts b/packages/babel-plugin-transform-modules-systemjs/src/index.ts
@@ -4,8 +4,9 @@ import { template, types as t } from "@babel/core";
 import { getImportSource } from "babel-plugin-dynamic-import-node/utils";
 import { rewriteThis, getModuleName } from "@babel/helper-module-transforms";
 import { isIdentifierName } from "@babel/helper-validator-identifier";
+import type { NodePath } from "@babel/traverse";
 
-const buildTemplate = template(`
+const buildTemplate = template.statement(`
   SYSTEM_REGISTER(MODULE_NAME, SOURCES, function (EXPORT_IDENTIFIER, CONTEXT_IDENTIFIER) {
     "use strict";
     BEFORE_BODY;
@@ -16,7 +17,7 @@ const buildTemplate = template(`
   });
 `);
 
-const buildExportAll = template(`
+const buildExportAll = template.statement(`
   for (var KEY in TARGET) {
     if (KEY !== "default" && KEY !== "__esModule") EXPORT_OBJ[KEY] = TARGET[KEY];
   }
@@ -286,7 +287,7 @@ export default declare((api, options) => {
             rewriteThis(path);
           }
         },
-        exit(path, state: PluginState) {
+        exit(path: NodePath<t.Program>, state: PluginState) {
           const scope = path.scope;
           const exportIdent = scope.generateUid("export");
           const { contextIdent, stringSpecifiers } = state;
@@ -332,7 +333,7 @@ export default declare((api, options) => {
           const exportNames = [];
           const exportValues = [];
 
-          const body: Array<any> = path.get("body");
+          const body = path.get("body");
 
           for (const path of body) {
             if (path.isFunctionDeclaration()) {
@@ -349,6 +350,10 @@ export default declare((api, options) => {
                   ),
                 ),
               );
+            } else if (path.isVariableDeclaration()) {
+              // Convert top-level variable declarations to "var",
+              // because they must be hoisted
+              path.node.kind = "var";
             } else if (path.isImportDeclaration()) {
               const source = path.node.source.value;
               pushModule(source, "imports", path.node.specifiers);
@@ -362,8 +367,8 @@ export default declare((api, options) => {
               path.remove();
             } else if (path.isExportDefaultDeclaration()) {
               const declar = path.get("declaration");
-              const id = declar.node.id;
               if (declar.isClassDeclaration()) {
+                const id = declar.node.id;
                 if (id) {
                   exportNames.push("default");
                   exportValues.push(scope.buildUndefinedNode());
@@ -384,6 +389,7 @@ export default declare((api, options) => {
                   removedPaths.push(path);
                 }
               } else if (declar.isFunctionDeclaration()) {
+                const id = declar.node.id;
                 if (id) {
                   beforeBody.push(declar.node);
                   exportNames.push("default");
@@ -403,15 +409,15 @@ export default declare((api, options) => {
               if (declar.node) {
                 path.replaceWith(declar);
 
-                if (path.isFunction()) {
+                if (declar.isFunction()) {
                   const node = declar.node;
                   const name = node.id.name;
                   addExportName(name, name);
                   beforeBody.push(node);
                   exportNames.push(name);
                   exportValues.push(t.cloneNode(node.id));
                   removedPaths.push(path);
-                } else if (path.isClass()) {
+                } else if (declar.isClass()) {
                   const name = declar.node.id.name;
                   exportNames.push(name);
                   exportValues.push(scope.buildUndefinedNode());
@@ -427,6 +433,11 @@ export default declare((api, options) => {
                   );
                   addExportName(name, name);
                 } else {
+                  if (declar.isVariableDeclaration()) {
+                    // Convert top-level variable declarations to "var",
+                    // because they must be hoisted
+                    declar.node.kind = "var";
+                  }
                   for (const name of Object.keys(
                     declar.getBindingIdentifiers(),
                   )) {
@@ -443,7 +454,10 @@ export default declare((api, options) => {
                     const nodes = [];
 
                     for (const specifier of specifiers) {
+                      // @ts-expect-error This isn't an "export ... from" declaration
+                      // because path.node.source is falsy, so the local specifier exists.
                       const { local, exported } = specifier;
+
                       const binding = scope.getBinding(local.name);
                       const exportedName = getExportSpecifierName(
                         exported,
@@ -565,19 +579,15 @@ export default declare((api, options) => {
           // @ts-expect-error todo(flow->ts): do not reuse variables
           if (moduleName) moduleName = t.stringLiteral(moduleName);
 
-          hoistVariables(
-            path,
-            (id, name, hasInit) => {
-              variableIds.push(id);
-              if (!hasInit && name in exportMap) {
-                for (const exported of exportMap[name]) {
-                  exportNames.push(exported);
-                  exportValues.push(scope.buildUndefinedNode());
-                }
+          hoistVariables(path, (id, name, hasInit) => {
+            variableIds.push(id);
+            if (!hasInit && name in exportMap) {
+              for (const exported of exportMap[name]) {
+                exportNames.push(exported);
+                exportValues.push(scope.buildUndefinedNode());
               }
-            },
-            null,
-          );
+            }
+          });
 
           if (variableIds.length) {
             beforeBody.unshift(
@@ -620,6 +630,7 @@ export default declare((api, options) => {
             Function(path) {
               path.skip();
             },
+            // @ts-expect-error - todo: add noScope to type definitions
             noScope: true,
           });
 
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-fn-decl/output.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-fn-decl/output.mjs
@@ -15,4 +15,4 @@ System.register([], function (_export, _context) {
       _export("testProp", testProp = 'test property');
     }
   };
-});
+});
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-let-const/input.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-let-const/input.mjs
@@ -0,0 +1,9 @@
+let l_foo = 1;
+const c_foo = 2;
+
+{ let l_foo, l_bar; const c_foo = 3; const c_bar = 4; }
+
+export { l_foo, c_foo };
+
+export let l_bar = 4;
+export const c_bar = 6;
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-let-const/output.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/export-let-const/output.mjs
@@ -0,0 +1,23 @@
+System.register([], function (_export, _context) {
+  "use strict";
+
+  var l_foo, c_foo, l_bar, c_bar;
+  return {
+    setters: [],
+    execute: function () {
+      _export("l_foo", l_foo = 1);
+
+      _export("c_foo", c_foo = 2);
+
+      {
+        let l_foo, l_bar;
+        const c_foo = 3;
+        const c_bar = 4;
+      }
+
+      _export("l_bar", l_bar = 4);
+
+      _export("c_bar", c_bar = 6);
+    }
+  };
+});
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/input.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/input.mjs
@@ -0,0 +1,8 @@
+import { x } from './x.js';
+
+if (true) {
+	const { x } = { x: 1 };
+	console.log(x);
+}
+
+new (class extends x {})();
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/output.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable-destructuring/output.mjs
@@ -0,0 +1,22 @@
+System.register(["./x.js"], function (_export, _context) {
+  "use strict";
+
+  var x;
+  return {
+    setters: [function (_xJs) {
+      x = _xJs.x;
+    }],
+    execute: function () {
+      if (true) {
+        const {
+          x
+        } = {
+          x: 1
+        };
+        console.log(x);
+      }
+
+      new class extends x {}();
+    }
+  };
+});
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/input.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/input.mjs
@@ -0,0 +1,8 @@
+import { x } from './x.js';
+
+if (true) {
+	const x = 1;
+	console.log(x);
+}
+
+new (class extends x {})();
diff --git a/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/output.mjs b/packages/babel-plugin-transform-modules-systemjs/test/fixtures/systemjs/module-level-variable/output.mjs
@@ -0,0 +1,18 @@
+System.register(["./x.js"], function (_export, _context) {
+  "use strict";
+
+  var x;
+  return {
+    setters: [function (_xJs) {
+      x = _xJs.x;
+    }],
+    execute: function () {
+      if (true) {
+        const x = 1;
+        console.log(x);
+      }
+
+      new class extends x {}();
+    }
+  };
+});

Original file line number	Diff line number	Diff line change
`@@ -15,4 +15,4 @@ System.register([], function (_export, _context) {`
`15`	`15`	`_export("testProp", testProp = 'test property');`
`16`	`16`	`}`
`17`	`17`	`};`
`18`		`-});`
	`18`	`+});`