Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run _createClass with object literals to allow property name mangling #10632

Open
ashi009 opened this issue Nov 3, 2019 · 10 comments

Comments

@ashi009
Copy link

@ashi009 ashi009 commented Nov 3, 2019

Feature Request

Is your feature request related to a problem? Please describe.

Assuming a simple pipeline like babel -> terser. When transfrom-classes is enabled, the generated code won't be able to be mangled by terser with mangle.properties option turned on.

In the transformed code _createClass is called with an array of descriptors to declare, and each descriptor of them has a key field with its original name. As it's a plain string, manglers won't treat it as an object property. Once mangled, the caller side of the code will use the mangled property name, yet the declaring side will use the original name, and the code just won't work.

Describe the solution you'd like
A clear and concise description of what you want to happen. Add any considered drawbacks.

instead of calling _createClass with an array of descriptors, use an object literal instead:

_createClass(Value, {
  method: {
    value: function method(name, error, value) {
      //...
    }
  }
}, {})

Describe alternatives you've considered

  • Set up a different pipeline to mangle first then to babel.
    It's less than ideal. To generate the minimized output, mangler needs to know the word frequency first, which needs babel to do its magic first.
  • Teach mangler to mangle keys
    By adding some magic comments like /*#__PURE__*/ to the keys, and teach manglers to treat them as property names. This is really a long shot, as there are so many tools around, and it will take ages to make the new magic string "de facto".

Teachability, Documentation, Adoption, Migration Strategy
Ideally, it should be a transparent change to the users, as this only changes the behavior of a plugin.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

@babel-bot babel-bot commented Nov 3, 2019

Hey @ashi009! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Nov 4, 2019

Can you post a comparison of the mangled result for the current output and for the proposed output?

@ashi009

This comment has been minimized.

Copy link
Author

@ashi009 ashi009 commented Nov 5, 2019

Sure thing.

Sample class:

export class A {
  methodA() {}
  methodB() {
   	console.log(this.methodA());
  }
}

The following snippets are the diffs of the current implementation and the patched one.

After Babel (repl):

@@ -9,7 +9,7 @@ function _instanceof(left, right) { if (right != null && typeof Symbol !== "unde
 
 function _classCallCheck(instance, Constructor) { if (!_instanceof(instance, Constructor)) { throw new TypeError("Cannot call a class as a function"); } }
 
-function _defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } }
+function _defineProperties(target, props) { for (var i in props) if (Object.prototype.hasOwnProperty.call(props, i)) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, i, descriptor); } }
 
 function _createClass(Constructor, protoProps, staticProps) { if (protoProps) _defineProperties(Constructor.prototype, protoProps); if (staticProps) _defineProperties(Constructor, staticProps); return Constructor; }
 
@@ -20,15 +20,16 @@ var A =
       _classCallCheck(this, A);
     }
 
-    _createClass(A, [{
-      key: "methodA",
-      value: function methodA() { }
-    }, {
-      key: "methodB",
-      value: function methodB() {
-        console.log(this.methodA());
+    _createClass(A, {
+      methodA: {
+        value: function methodA() { }
+      },
+      methodB: {
+        value: function methodB() {
+          console.log(this.methodA());
+        }
       }
-    }]);
+    });
 
     return A;
   }();

Optimized with Terser (using a long list of reserved property names):

@@ -9,10 +9,10 @@ function _classCallCheck(e, n) {
 }
 
 function _defineProperties(e, n) {
-    for (var t = 0; t < n.e; t++) {
+    for (var t in n) if (Object.prototype.hasOwnProperty.call(n, t)) {
         var o = n[t];
         o.enumerable = o.enumerable || !1, o.configurable = !0, "value" in o && (o.writable = !0), 
-        Object.defineProperty(e, o.n, o);
+        Object.defineProperty(e, t, o);
     }
 }
@@ -28,15 +28,16 @@ var A = function() {
     function e() {
         _classCallCheck(this, e);
     }
-    return _createClass(e, [ {
-        n: "methodA",
-        value: function() {}
-    }, {
-        n: "methodB",
-        value: function() {
-            console.log(this.o());  // <-- `this.o` is undefined
+    return _createClass(e, {
+        n: {
+            value: function() {}
+        },
+        t: {
+            value: function() {
+                console.log(this.n());  // <-- `this.n` is defined above
+            }
         }
-    } ]), e;
+    }), e;
 }();
 
 exports.A = A;

Optimize with Closure Compiler:

@@ -2,12 +2,15 @@ Object.defineProperty(exports, "__esModule", { value: !0 });
 exports.a = void 0;
 function e(a, c) {
   if (c) {
-    for (var f = a.prototype, d = 0; d < c.length; d++) {
-      var b = c[d];
-      b.enumerable = b.enumerable || !1;
-      b.configurable = !0;
-      "value" in b && (b.writable = !0);
-      Object.defineProperty(f, b.key, b);
+    var f = a.prototype, d;
+    for (d in c) {
+      if (Object.prototype.hasOwnProperty.call(c, d)) {
+        var b = c[d];
+        b.enumerable = b.enumerable || !1;
+        b.configurable = !0;
+        "value" in b && (b.writable = !0);
+        Object.defineProperty(f, d, b);
+      }
     }
   }
 }
@@ -17,13 +20,16 @@ var g = function () {
       throw new TypeError("Cannot call a class as a function");
     }
   }
-  e(a, [{
-    key: "methodA", value: function () { }
-  }, {
-    key: "methodB", value: function () {
-      console.log(this.b());  // <-- `this.b` is undefined.
+  e(a, {
+    b: {
+      value: function () {
+      }
+    }, c: {
+      value: function () {
+        console.log(this.b());  // <-- `this.b` is defined above.
+      }
     }
-  }]);
+  });
   return a;
 }();
 exports.a = g;
@jridgewell

This comment has been minimized.

Copy link
Member

@jridgewell jridgewell commented Nov 5, 2019

The issue with this is we'll have to use Object.getOwnPropertySymbols to get any symbol'd keys. Otherwise, this is fine.

@JLHwung

This comment has been minimized.

Copy link
Contributor

@JLHwung JLHwung commented Nov 5, 2019

We may replace { key: Identifier, value: function() } by [Identifier, function()] without changing the helper logic too much.

Note that any helper interface change is a breaking change unless we go with a new helper such as _defineProperty2.

@ashi009

This comment has been minimized.

Copy link
Author

@ashi009 ashi009 commented Nov 5, 2019

We may replace { key: Identifier, value: function() } by [Identifier, function()] without changing the helper logic too much.

Note that any helper interface change is a breaking change unless we go with a new helper such as _defineProperty2.

This won't work either. A plain string cannot be naturally interpreted as a property. Also, it will break getter/setter definitions.

@ashi009

This comment has been minimized.

Copy link
Author

@ashi009 ashi009 commented Nov 5, 2019

The issue with this is we'll have to use Object.getOwnPropertySymbols to get any symbol'd keys. Otherwise, this is fine.

Yep. Just out of curiosity, why not just call Object.defineProperties() with all the descriptors? Which already handles symbol'd keys.

@loganfsmyth

This comment has been minimized.

Copy link
Member

@loganfsmyth loganfsmyth commented Nov 5, 2019

Personally, I'd have a lot of reservations about this. Even if we change this one specific case, what about other transforms? For instance

var { first, ...rest } = {};

becomes

var _ref = {},
    first = _ref.first,
    rest = _objectWithoutProperties(_ref, ["first"]);

Do we also need to handle mangling of first too?

What happens if there are situations where we are unable to transform code in such a way that a mangler can tell what to do? For decorators for instance, it has at times been important that individual class elements evaluate in order.

I worry that aiming to support this partially is still just as bad.

Babel hasn't generally aimed to make any guarantees about the output of plugins other than final runtime behavior, so changing that would be an increase in the surface area where we need to be concerned about breakage for any change we make.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Nov 5, 2019

Also, it wouldn't work with this code:

function getX() { return "x" }
function getX2() { return "x" }

class A {
  get [getX()]() {}
  set [getX2()](_) {}
}

Since using an object the set would overwrite the get instead of extending it.

@ashi009

This comment has been minimized.

Copy link
Author

@ashi009 ashi009 commented Nov 5, 2019

@loganfsmyth Sounds fair.

Property name mangling has been a very challenging problem to tackle, given the dynamic nature of js. However, anyone or any company that wants to do this will write their code against some strict style guidelines (like Google's), and even willing to avoid using certain language features.

My team writes code carefully to allow code optimizers to do its magic, and so far it only works if no downlevel was made to the code. As babel is probably one of the best open-source transpiler available, we hope that transforms done to the AST are preserving those nuances.

For the rest transform, it can be easily done preserving the property nature:

var _ref = {},
    first = _ref.first,
    rest = _objectWithoutProperties(_ref, {first:true});

This is indeed a longshot, but I think it's totally doable, as long as we add tests to cover those cases. (Just think about how the sideEffects flag being adopted to make tree-shaking possible without AST analysis.)

@nicolo-ribaudo Yes, my demo code won't work for your case. But it's just a corner case to be covered in real code. For instance, by making _createClass take an array of prop descriptors:

_createClass(Ctor, [
  {
    method1: {},
    method2: {},
    [getX()]: {},
  }, {
    [getX2()]: {},
  }, {
    method3: {}
    [getX3()]: {},
  },
]);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.