Handle ClassDeclaration binding (#383)

* Handle ClassDeclaration binding bug in babel + (Fixes #365) This is happening because ClassDeclarations have bindings in two scopes where as FunctionDeclarations have bindings only in the scope where it is defined. + babel bug: babel/babel#5156 * Fix comment * New approach to topLevel and classDecl
babel · Feb 16, 2017 · e836e60 · e836e60
1 parent 8b86de0
commit e836e60
Show file tree

Hide file tree

Showing 2 changed files with 95 additions and 84 deletions.
diff --git a/packages/babel-plugin-minify-mangle-names/__tests__/mangle-names-test.js b/packages/babel-plugin-minify-mangle-names/__tests__/mangle-names-test.js
@@ -1129,4 +1129,24 @@ describe("mangle-names", () => {
     `);
     expect(transform(source)).toBe(expected);
   });
+
+  it("should fix issue#365 - classDeclaration with unsafe parent scope", () => {
+    const source = unpad(`
+      function foo() {
+        eval("");
+        class A {}
+        class B {}
+      }
+    `);
+    expect(transform(source)).toBe(source);
+  });
+
+  it("should fix classDeclaration with unsafe program scope", () => {
+    const source = unpad(`
+      class A {}
+      class B {}
+      eval("");
+    `);
+    expect(transform(source, { topLevel: true })).toBe(source);
+  });
 });
diff --git a/packages/babel-plugin-minify-mangle-names/src/index.js b/packages/babel-plugin-minify-mangle-names/src/index.js
@@ -86,100 +86,91 @@ module.exports = ({ types: t, traverse }) => {
       }
     }
 
-    mangle() {
+    mangleScope(scope) {
       const mangler = this;
 
-      this.program.traverse({
-        Scopable(path) {
-          const {scope} = path;
+      if (!mangler.eval && hasEval(scope)) return;
 
-          if (!mangler.eval && hasEval(scope)) return;
+      if (mangler.visitedScopes.has(scope)) return;
+      mangler.visitedScopes.add(scope);
 
-          if (mangler.visitedScopes.has(scope)) return;
-          mangler.visitedScopes.add(scope);
+      let i = 0;
+      function getNext() {
+        return mangler.charset.getIdentifier(i++);
+      }
 
-          function hasOwnBinding(name) {
-            if (scope.parent !== mangler.program.scope) {
-              return scope.hasOwnBinding(name);
-            }
-            return mangler.program.scope.hasOwnBinding(name) || scope.hasOwnBinding(name);
-          }
+      // This is useful when we have vars of single character
+      // => var a, ...z, A, ...Z, $, _;
+      // to
+      // => var aa, a, b ,c;
+      // instead of
+      // => var aa, ab, ...;
+      // TODO:
+      // Re-enable after enabling this feature
+      // This doesn't work right now as we are concentrating
+      // on performance improvements
+      // function resetNext() {
+      //   i = 0;
+      // }
+
+      const bindings = scope.getAllBindings();
+      const names = Object.keys(bindings);
+
+      for (let i = 0; i < names.length; i++) {
+        const oldName = names[i];
+        const binding = bindings[oldName];
+
+        if (
+          // arguments
+          oldName === "arguments"
+          // other scope bindings
+          || !scope.hasOwnBinding(oldName)
+          // labels
+          || binding.path.isLabeledStatement()
+          // ClassDeclaration has binding in two scopes
+          //   1. The scope in which it is declared
+          //   2. The class's own scope
+          // - https://github.com/babel/babel/issues/5156
+          || (binding.path.isClassDeclaration() && binding.path === scope.path)
+          // blacklisted
+          || mangler.isBlacklist(oldName)
+          // function names
+          || (mangler.keepFnName ? isFunction(binding.path) : false)
+          // class names
+          || (mangler.keepClassName ? isClass(binding.path) : false)
+        ) {
+          continue;
+        }
 
-          let i = 0;
-          function getNext() {
-            return mangler.charset.getIdentifier(i++);
-          }
+        let next;
+        do {
+          next = getNext();
+        } while (
+          !t.isValidIdentifier(next)
+          || hop.call(bindings, next)
+          || scope.hasGlobal(next)
+          || scope.hasReference(next)
+        );
+
+        // TODO:
+        // re-enable this - check above
+        // resetNext();
+        mangler.rename(scope, oldName, next);
+      }
+    }
 
-          // This is useful when we have vars of single character
-          // => var a, ...z, A, ...Z, $, _;
-          // to
-          // => var aa, a, b ,c;
-          // instead of
-          // => var aa, ab, ...;
-          // TODO:
-          // Re-enable after enabling this feature
-          // This doesn't work right now as we are concentrating
-          // on performance improvements
-          // function resetNext() {
-          //   i = 0;
-          // }
-
-          const bindings = scope.getAllBindings();
-          const names = Object.keys(bindings);
-
-          for (let i = 0; i < names.length; i++) {
-            const oldName = names[i];
-            const binding = bindings[oldName];
-            const isTopLevel = mangler.program.scope.bindings[oldName] === binding;
-
-            if (
-              // already renamed bindings
-              binding.renamed
-              // arguments
-              || oldName === "arguments"
-              // globals
-              || (mangler.topLevel ? false : isTopLevel)
-              // other scope bindings
-              || !hasOwnBinding(oldName)
-              // labels
-              || binding.path.isLabeledStatement()
-              // blacklisted
-              || mangler.isBlacklist(oldName)
-              // function names
-              || (mangler.keepFnName ? isFunction(binding.path) : false)
-              // class names
-              || (mangler.keepClassName ? isClass(binding.path) : false)
-            ) {
-              continue;
-            }
+    mangle() {
+      const mangler = this;
 
-            let next;
-            do {
-              next = getNext();
-            } while (
-              !t.isValidIdentifier(next)
-              || hop.call(bindings, next)
-              || scope.hasGlobal(next)
-              || scope.hasReference(next)
-            );
-
-            // TODO:
-            // re-enable this - check above
-            // resetNext();
-            mangler.rename(scope, oldName, next);
-            if (isTopLevel) {
-              mangler.rename(mangler.program.scope, oldName, next);
-            }
-            // mark the binding as renamed
-            binding.renamed = true;
-          }
+      if (mangler.topLevel) {
+        mangler.mangleScope(mangler.program.scope);
+      }
+
+      this.program.traverse({
+        Scopable(path) {
+          mangler.mangleScope(path.scope);
         }
       });
-
-      // TODO:
-      // re-enable
-      // check above
-      // this.updateReferences();
     }
 
     rename(scope, oldName, newName) {