fix: Computed properties should keep original definition order (#15232)

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com> Co-authored-by: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Fixes #15140
babel · Dec 5, 2022 · e5e1369 · e5e1369
1 parent 2bba4a5
commit e5e1369
Show file tree

Hide file tree

Showing 22 changed files with 188 additions and 82 deletions.
diff --git a/Makefile b/Makefile
@@ -189,15 +189,15 @@ test-test262-update-allowlist:
 
 
 new-version-checklist:
-	# @echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-	# @echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-	# @echo "!!!!!!                                                   !!!!!!"
-	# @echo "!!!!!! Add any message here, and UNCOMMENT THESE LINES!  !!!!!!"
-	# @echo "!!!!!!                                                   !!!!!!"
-	# @echo "!!!!!!                                                   !!!!!!"
-	# @echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-	# @echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
-	# @exit 1
+	@echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
+	@echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
+	@echo "!!!!!!                                                      !!!!!!"
+	@echo "!!!!!!               Update the minVersion of               !!!!!!"
+	@echo "!!!!!! packages/babel-helpers/src/helpers/defineAccessor.js !!!!!!"
+	@echo "!!!!!!                                                      !!!!!!"
+	@echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
+	@echo "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
+	@exit 1
 
 new-version:
 	$(MAKE) new-version-checklist

diff --git a/packages/babel-core/src/transformation/file/file.ts b/packages/babel-core/src/transformation/file/file.ts
@@ -210,7 +210,7 @@ export default class File {
     });
 
     nodes.forEach(node => {
-      // @ts-expect-error Fixeme: document _compact node property
+      // @ts-expect-error Fixme: document _compact node property
       node._compact = true;
     });
 

diff --git a/packages/babel-helpers/src/helpers-generated.ts b/packages/babel-helpers/src/helpers-generated.ts
@@ -45,6 +45,10 @@ export default Object.freeze({
     "7.20.5",
     'export default function _checkInRHS(value){if(Object(value)!==value)throw TypeError("right-hand side of \'in\' should be an object, got "+(null!==value?typeof value:"null"));return value}',
   ),
+  defineAccessor: helper(
+    "7.20.6",
+    "export default function _defineAccessor(type,obj,key,fn){var desc={configurable:!0,enumerable:!0};return desc[type]=fn,Object.defineProperty(obj,key,desc)}",
+  ),
   iterableToArrayLimit: helper(
     "7.0.0-beta.0",
     'export default function _iterableToArrayLimit(arr,i){var _i=null==arr?null:"undefined"!=typeof Symbol&&arr[Symbol.iterator]||arr["@@iterator"];if(null!=_i){var _s,_e,_x,_r,_arr=[],_n=!0,_d=!1;try{if(_x=(_i=_i.call(arr)).next,0===i){if(Object(_i)!==_i)return;_n=!1}else for(;!(_n=(_s=_x.call(_i)).done)&&(_arr.push(_s.value),_arr.length!==i);_n=!0);}catch(err){_d=!0,_e=err}finally{try{if(!_n&&null!=_i.return&&(_r=_i.return(),Object(_r)!==_r))return}finally{if(_d)throw _e}}return _arr}}',

diff --git a/packages/babel-helpers/src/helpers/defineAccessor.js b/packages/babel-helpers/src/helpers/defineAccessor.js
@@ -0,0 +1,8 @@
+/* @minVersion 7.20.6 */
+
+export default function _defineAccessor(type, obj, key, fn) {
+  var desc = { configurable: true, enumerable: true };
+  // type should be "get" or "set"
+  desc[type] = fn;
+  return Object.defineProperty(obj, key, desc);
+}
diff --git a/packages/babel-plugin-transform-computed-properties/package.json b/packages/babel-plugin-transform-computed-properties/package.json
@@ -17,7 +17,8 @@
     "babel-plugin"
   ],
   "dependencies": {
-    "@babel/helper-plugin-utils": "workspace:^"
+    "@babel/helper-plugin-utils": "workspace:^",
+    "@babel/template": "workspace:^"
   },
   "peerDependencies": {
     "@babel/core": "^7.0.0-0"

diff --git a/packages/babel-plugin-transform-computed-properties/src/index.ts b/packages/babel-plugin-transform-computed-properties/src/index.ts
@@ -1,5 +1,7 @@
+import { types as t } from "@babel/core";
+import type { PluginPass } from "@babel/core";
 import { declare } from "@babel/helper-plugin-utils";
-import { template, types as t, type PluginPass } from "@babel/core";
+import template from "@babel/template";
 import type { Scope } from "@babel/traverse";
 
 export interface Options {
@@ -12,10 +14,19 @@ type PropertyInfo = {
   body: t.Statement[];
   computedProps: t.ObjectMember[];
   initPropExpression: t.ObjectExpression;
-  getMutatorId: () => t.Identifier;
   state: PluginPass;
 };
 
+// TODO(Babel 8): Remove this
+const DefineAccessorHelper = template.expression.ast`
+function (type, obj, key, fn) {
+  var desc = { configurable: true, enumerable: true };
+  desc[type] = fn;
+  return Object.defineProperty(obj, key, desc);
+}`;
+// @ts-expect-error undocumented _compact node property
+DefineAccessorHelper._compact = true;
+
 export default declare((api, options: Options) => {
   api.assertVersion(7);
 
@@ -26,10 +37,34 @@ export default declare((api, options: Options) => {
     ? pushComputedPropsLoose
     : pushComputedPropsSpec;
 
-  const buildMutatorMapAssign = template.statements(`
-    MUTATOR_MAP_REF[KEY] = MUTATOR_MAP_REF[KEY] || {};
-    MUTATOR_MAP_REF[KEY].KIND = VALUE;
-  `);
+  function buildDefineAccessor(
+    state: PluginPass,
+    type: "get" | "set",
+    obj: t.Expression,
+    key: t.Expression,
+    fn: t.Expression,
+  ) {
+    let helper: t.Identifier;
+    if (state.availableHelper("defineAccessor")) {
+      helper = state.addHelper("defineAccessor");
+    } else {
+      // Fallback for @babel/helpers <= 7.20.6, manually add helper function
+      // TODO(Babel 8): Remove this
+      const file = state.file;
+      helper = file.get("fallbackDefineAccessorHelper");
+      if (!helper) {
+        const id = file.scope.generateUidIdentifier("defineAccessor");
+        file.scope.push({
+          id,
+          init: DefineAccessorHelper,
+        });
+        file.set("fallbackDefineAccessorHelper", (helper = id));
+      }
+      helper = t.cloneNode(helper);
+    }
+
+    return t.callExpression(helper, [t.stringLiteral(type), obj, key, fn]);
+  }
 
   /**
    * Get value of an object member under object expression.
@@ -72,31 +107,26 @@ export default declare((api, options: Options) => {
     );
   }
 
-  function pushMutatorDefine(
-    { body, getMutatorId, scope }: PropertyInfo,
+  function pushAccessorDefine(
+    { body, computedProps, initPropExpression, objId, state }: PropertyInfo,
     prop: t.ObjectMethod,
   ) {
-    let key =
+    const kind = prop.kind as "get" | "set";
+    const key =
       !prop.computed && t.isIdentifier(prop.key)
         ? t.stringLiteral(prop.key.name)
         : prop.key;
+    const value = getValue(prop);
 
-    const maybeMemoise = scope.maybeGenerateMemoised(key);
-    if (maybeMemoise) {
+    if (computedProps.length === 1) {
+      return buildDefineAccessor(state, kind, initPropExpression, key, value);
+    } else {
       body.push(
-        t.expressionStatement(t.assignmentExpression("=", maybeMemoise, key)),
+        t.expressionStatement(
+          buildDefineAccessor(state, kind, t.cloneNode(objId), key, value),
+        ),
       );
-      key = maybeMemoise;
     }
-
-    body.push(
-      ...buildMutatorMapAssign({
-        MUTATOR_MAP_REF: getMutatorId(),
-        KEY: t.cloneNode(key),
-        VALUE: getValue(prop),
-        KIND: t.identifier(prop.kind),
-      }),
-    );
   }
 
   function pushComputedPropsLoose(info: PropertyInfo) {
@@ -105,7 +135,8 @@ export default declare((api, options: Options) => {
         t.isObjectMethod(prop) &&
         (prop.kind === "get" || prop.kind === "set")
       ) {
-        pushMutatorDefine(info, prop);
+        const single = pushAccessorDefine(info, prop);
+        if (single) return single;
       } else {
         pushAssign(t.cloneNode(info.objId), prop, info.body);
       }
@@ -123,7 +154,8 @@ export default declare((api, options: Options) => {
         t.isObjectMethod(prop) &&
         (prop.kind === "get" || prop.kind === "set")
       ) {
-        pushMutatorDefine(info, prop);
+        const single = pushAccessorDefine(info, prop);
+        if (single) return single;
       } else {
         // the value of ObjectProperty in ObjectExpression must be an expression
         const value = getValue(prop) as t.Expression;
@@ -195,44 +227,15 @@ export default declare((api, options: Options) => {
             ]),
           );
 
-          let mutatorRef: t.Identifier;
-
-          const getMutatorId = function () {
-            if (!mutatorRef) {
-              mutatorRef = scope.generateUidIdentifier("mutatorMap");
-
-              body.push(
-                t.variableDeclaration("var", [
-                  t.variableDeclarator(mutatorRef, t.objectExpression([])),
-                ]),
-              );
-            }
-
-            return t.cloneNode(mutatorRef);
-          };
-
           const single = pushComputedProps({
             scope,
             objId,
             body,
             computedProps,
             initPropExpression,
-            getMutatorId,
             state,
           });
 
-          if (mutatorRef) {
-            body.push(
-              t.expressionStatement(
-                t.callExpression(
-                  state.addHelper("defineEnumerableProperties"),
-                  [t.cloneNode(objId), t.cloneNode(mutatorRef)],
-                ),
-              ),
-            );
-          }
-
-          // @ts-expect-error todo(flow->ts) `void` should not be used as variable
           if (single) {
             path.replaceWith(single);
           } else {

diff --git a/...rm-computed-properties/test/fixtures/assumption-setComputedProperties/accessors/output.js b/...rm-computed-properties/test/fixtures/assumption-setComputedProperties/accessors/output.js
@@ -1,10 +1,10 @@
-var _foobar, _foobar2, _test, _test2, _obj, _mutatorMap;
-var obj = (_obj = {}, _foobar = foobar, _mutatorMap = {}, _mutatorMap[_foobar] = _mutatorMap[_foobar] || {}, _mutatorMap[_foobar].get = function () {
+var _obj;
+var obj = (_obj = {}, babelHelpers.defineAccessor("get", _obj, foobar, function () {
   return "foobar";
-}, _foobar2 = foobar, _mutatorMap[_foobar2] = _mutatorMap[_foobar2] || {}, _mutatorMap[_foobar2].set = function (x) {
+}), babelHelpers.defineAccessor("set", _obj, foobar, function (x) {
   console.log(x);
-}, _test = "test", _mutatorMap[_test] = _mutatorMap[_test] || {}, _mutatorMap[_test].get = function () {
+}), babelHelpers.defineAccessor("get", _obj, "test", function () {
   return "regular getter after computed property";
-}, _test2 = "test", _mutatorMap[_test2] = _mutatorMap[_test2] || {}, _mutatorMap[_test2].set = function (x) {
+}), babelHelpers.defineAccessor("set", _obj, "test", function (x) {
   console.log(x);
-}, babelHelpers.defineEnumerableProperties(_obj, _mutatorMap), _obj);
+}), _obj);
diff --git a/...mputed-properties/test/fixtures/assumption-setComputedProperties/single-accessor/input.js b/...mputed-properties/test/fixtures/assumption-setComputedProperties/single-accessor/input.js
@@ -0,0 +1,3 @@
+var obj = {
+  get ["x" + foo]() { return "heh"; }
+};
diff --git a/...puted-properties/test/fixtures/assumption-setComputedProperties/single-accessor/output.js b/...puted-properties/test/fixtures/assumption-setComputedProperties/single-accessor/output.js
@@ -0,0 +1,3 @@
+var obj = babelHelpers.defineAccessor("get", {}, "x" + foo, function () {
+  return "heh";
+});
diff --git a/...sform-computed-properties/test/fixtures/assumption-setComputedProperties/symbol/output.js b/...sform-computed-properties/test/fixtures/assumption-setComputedProperties/symbol/output.js
@@ -1,5 +1,5 @@
-var _foo, _mutatorMap;
+var _foo;
 var k = Symbol();
-var foo = (_foo = {}, _foo[Symbol.iterator] = "foobar", _mutatorMap = {}, _mutatorMap[k] = _mutatorMap[k] || {}, _mutatorMap[k].get = function () {
+var foo = (_foo = {}, _foo[Symbol.iterator] = "foobar", babelHelpers.defineAccessor("get", _foo, k, function () {
   return k;
-}, babelHelpers.defineEnumerableProperties(_foo, _mutatorMap), _foo);
+}), _foo);
diff --git a/packages/babel-plugin-transform-computed-properties/test/fixtures/spec/accessors/output.js b/packages/babel-plugin-transform-computed-properties/test/fixtures/spec/accessors/output.js
@@ -1,10 +1,10 @@
-var _foobar, _foobar2, _test, _test2, _obj, _mutatorMap;
-var obj = (_obj = {}, _foobar = foobar, _mutatorMap = {}, _mutatorMap[_foobar] = _mutatorMap[_foobar] || {}, _mutatorMap[_foobar].get = function () {
+var _obj;
+var obj = (_obj = {}, babelHelpers.defineAccessor("get", _obj, foobar, function () {
   return "foobar";
-}, _foobar2 = foobar, _mutatorMap[_foobar2] = _mutatorMap[_foobar2] || {}, _mutatorMap[_foobar2].set = function (x) {
+}), babelHelpers.defineAccessor("set", _obj, foobar, function (x) {
   console.log(x);
-}, _test = "test", _mutatorMap[_test] = _mutatorMap[_test] || {}, _mutatorMap[_test].get = function () {
+}), babelHelpers.defineAccessor("get", _obj, "test", function () {
   return "regular getter after computed property";
-}, _test2 = "test", _mutatorMap[_test2] = _mutatorMap[_test2] || {}, _mutatorMap[_test2].set = function (x) {
+}), babelHelpers.defineAccessor("set", _obj, "test", function (x) {
   console.log(x);
-}, babelHelpers.defineEnumerableProperties(_obj, _mutatorMap), _obj);
+}), _obj);
diff --git a/...es/babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/exec.js b/...es/babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/exec.js
@@ -0,0 +1,17 @@
+var a = {
+  get ["x"]() { return 0; },
+  ["y"]: 1,
+};
+expect(Object.keys(a)).toStrictEqual(["x", "y"]);
+
+var b = {
+  get ["x"]() { return 0; },
+  ["x"]: 1,
+};
+expect(b.x).toBe(1);
+
+var x = 1;
+var y = { x, get x() { return 0; }, x };
+expect(y.x).toBe(1);
+y.x = 2;
+expect(y.x).toBe(2);
diff --git a/...s/babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/input.js b/...s/babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/input.js
@@ -0,0 +1,13 @@
+var a = {
+  get ["x"]() { return 0; },
+  ["y"]: 1,
+};
+
+var b = {
+  get ["x"]() { return 0; },
+  ["x"]: 1,
+};
+
+var x = 1;
+var y = { x, get x() { return 0; }, x };
+y.x = 2;
diff --git a/...bel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/options.json b/...bel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/options.json
@@ -0,0 +1,6 @@
+{
+  "plugins": [
+    "transform-duplicate-keys",
+    "transform-computed-properties"
+  ]
+}
diff --git a/.../babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/output.js b/.../babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order/output.js
@@ -0,0 +1,14 @@
+var _a, _b, _y;
+var a = (_a = {}, babelHelpers.defineAccessor("get", _a, "x", function () {
+  return 0;
+}), babelHelpers.defineProperty(_a, "y", 1), _a);
+var b = (_b = {}, babelHelpers.defineAccessor("get", _b, "x", function () {
+  return 0;
+}), babelHelpers.defineProperty(_b, "x", 1), _b);
+var x = 1;
+var y = (_y = {
+  x
+}, babelHelpers.defineAccessor("get", _y, "x", function () {
+  return 0;
+}), babelHelpers.defineProperty(_y, "x", x), _y);
+y.x = 2;
diff --git a/...es/babel-plugin-transform-computed-properties/test/fixtures/spec/single-accessor/input.js b/...es/babel-plugin-transform-computed-properties/test/fixtures/spec/single-accessor/input.js
@@ -0,0 +1,3 @@
+var obj = {
+  get ["x" + foo]() { return "heh"; }
+};
diff --git a/...s/babel-plugin-transform-computed-properties/test/fixtures/spec/single-accessor/output.js b/...s/babel-plugin-transform-computed-properties/test/fixtures/spec/single-accessor/output.js
@@ -0,0 +1,3 @@
+var obj = babelHelpers.defineAccessor("get", {}, "x" + foo, function () {
+  return "heh";
+});
diff --git a/packages/babel-plugin-transform-computed-properties/test/fixtures/spec/symbol/output.js b/packages/babel-plugin-transform-computed-properties/test/fixtures/spec/symbol/output.js
@@ -1,5 +1,5 @@
-var _foo, _mutatorMap;
+var _foo;
 var k = Symbol();
-var foo = (_foo = {}, babelHelpers.defineProperty(_foo, Symbol.iterator, "foobar"), _mutatorMap = {}, _mutatorMap[k] = _mutatorMap[k] || {}, _mutatorMap[k].get = function () {
+var foo = (_foo = {}, babelHelpers.defineProperty(_foo, Symbol.iterator, "foobar"), babelHelpers.defineAccessor("get", _foo, k, function () {
   return k;
-}, babelHelpers.defineEnumerableProperties(_foo, _mutatorMap), _foo);
+}), _foo);
diff --git a/packages/babel-runtime-corejs2/package.json b/packages/babel-runtime-corejs2/package.json
@@ -90,6 +90,15 @@
       "./helpers/checkInRHS.js"
     ],
     "./helpers/esm/checkInRHS": "./helpers/esm/checkInRHS.js",
+    "./helpers/defineAccessor": [
+      {
+        "node": "./helpers/defineAccessor.js",
+        "import": "./helpers/esm/defineAccessor.js",
+        "default": "./helpers/defineAccessor.js"
+      },
+      "./helpers/defineAccessor.js"
+    ],
+    "./helpers/esm/defineAccessor": "./helpers/esm/defineAccessor.js",
     "./helpers/iterableToArrayLimit": [
       {
         "node": "./helpers/iterableToArrayLimit.js",

diff --git a/packages/babel-runtime-corejs3/package.json b/packages/babel-runtime-corejs3/package.json
@@ -89,6 +89,15 @@
       "./helpers/checkInRHS.js"
     ],
     "./helpers/esm/checkInRHS": "./helpers/esm/checkInRHS.js",
+    "./helpers/defineAccessor": [
+      {
+        "node": "./helpers/defineAccessor.js",
+        "import": "./helpers/esm/defineAccessor.js",
+        "default": "./helpers/defineAccessor.js"
+      },
+      "./helpers/defineAccessor.js"
+    ],
+    "./helpers/esm/defineAccessor": "./helpers/esm/defineAccessor.js",
     "./helpers/iterableToArrayLimit": [
       {
         "node": "./helpers/iterableToArrayLimit.js",