fix: Computed properties should keep original definition order

[SuperSodaSea]

Q	A
Fixed Issues?	Fixes #15140
Patch: Bug Fix?	👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	👍
License	MIT

@babel/plugin-transform-computed-properties use an _mutatorMap to hold the accessor computed properties in an object initializer, and use defineEnumerableProperties to define them at the end of initialization, which can change the definition order. This PR change it to use new helper function defineAccessor to define accessor computed properties in place, keeping the original definition order.

Tests

➕ Added

• babel-plugin-transform-computed-properties/test/fixtures/assumption-setComputedProperties/single-accessor
• babel-plugin-transform-computed-properties/test/fixtures/spec/definition-order
• babel-plugin-transform-computed-properties/test/fixtures/spec/single-accessor

〰️ Output Changed

• babel-plugin-transform-computed-properties/test/fixtures/assumption-setComputedProperties/accessors
• babel-plugin-transform-computed-properties/test/fixtures/assumption-setComputedProperties/symbol
• babel-plugin-transform-computed-properties/test/fixtures/spec/accessors
• babel-plugin-transform-computed-properties/test/fixtures/spec/symbol

[SuperSodaSea]

Not sure if this is the correct minVersion...

[nicolo-ribaudo]

We will need to update it when releasing, could you update the Makefile like I did in 80c6889?

[SuperSodaSea]

Done in 16758f3.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/53594/

[liuxingbaoyu]

packages/babel-plugin-transform-computed-properties/test/fixtures/regression/15140/exec.js

This doesn't appear to be a regression.

[SuperSodaSea]

Where should this test be?

[JLHwung]

spec/definition-ordering looks good to me, also loose/definition-ordering if it works.

[SuperSodaSea]

Oops, seems that I misunderstood the meaning of "regression". Then the name of the tests in my previous PR #15183 and #15194 may also be wrong...

[SuperSodaSea]

Now the test is renamed to spec/definition-order. It doesn't apply to loose or assumption-setComputedProperties.

[nicolo-ribaudo]

This is very nitpicky, the PR is very good as it is.

However, babelHelpers.defineAccessor("get", _obj, foobar, function () { gzips better, because gzip can deduplicate all the defineAccessor("get", parts in the code to refer to the same byte sequence. With defineAccessor(_obj, foobar, "get", the only constant part that gzip can easily deduplicate is only defineAccessor(, because the _obj variable will likely have a different name for different calls.

[SuperSodaSea]

Done in 1fd56a3.

[nicolo-ribaudo]

Nit, can you use two different variables?

var y = { x, get x() { return 0; }, x };

[SuperSodaSea]

Done in c735135.

[SuperSodaSea]

Oh, I found a problem: using @babel/plugin-transform-computed-properties without @babel/plugin-transform-spread will silently remove the spread in object initializer (REPL). Maybe we need to throw an Error in this situation.

[nicolo-ribaudo]

Could you open a separate PR for that? 🙏

[liuxingbaoyu]

Just confused: I noticed a new helper was added, do we need to maintain compatibility without the new helper? @nicolo-ribaudo

[nicolo-ribaudo]

@liuxingbaoyu You are right!

@SuperSodaSea When using the new version of this plugin with older @babel/core/@babel/helpers versions, the new helper won't be available and we need a fallback.

We could do something like this:

function buildHelperCall(file, type, obj, name, fn) {
  if (file.availableHelper("defineAccessor")) {
    return t.callExpression(file.addHelper("defineAccessor"), [type, obj, name, fn]));
  }

  // Fallback for @babel/helpers <= 7.20.6
  return template.expression.ast`
    Object.defineProperty(${obj}, ${name}, {
      ${type}: ${fn},
      enumerable: true,
      configurable: true
    })
  `;
}

We don't currently have a way to test this, other than manually by replacing the availableHelper call with just false.

[SuperSodaSea]

@nicolo-ribaudo

Could you open a separate PR for that? 🙏

I'll do that after this PR is merged.

When using the new version of this plugin with older @babel/core/@babel/helpers versions, the new helper won't be available and we need a fallback.

Or maybe we just don't need a new helper function and always use Object.defineProperty directly. What's your opinion?

[liuxingbaoyu]

Or maybe we just don't need a new helper function and always use Object.defineProperty directly. What's your opinion?

I'm concerned that this will slightly severely impact the output size.
Although this results in the user having to use a bundler, we already do.

Also I guess we can add helpers directly into the code? While this may result in a global variable, we currently have similar behavior and this should have no side effects.
path.scope.generateDeclaredUidIdentifier

[SuperSodaSea]

Now the helper function is directly added to the file scope if it is not available. For example:

var obj = {
  get ["x" + foo]() { return "heh"; }
};

The output would be:

- var obj = babelHelpers.defineAccessor("get", {}, "x" + foo, function () {
+ function _defineAccessor(type, obj, key, fn) {
+   var desc = {
+     configurable: true,
+     enumerable: true
+   };
+   desc[type] = fn;
+   return Object.defineProperty(obj, key, desc);
+ }
+ var obj = _defineAccessor("get", {}, "x" + foo, function () {
    return "heh";
  });

[liuxingbaoyu]

From ddf2251f435c6f3c7e622644ee2965183532f01a Mon Sep 17 00:00:00 2001
From: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com>
Date: Sat, 3 Dec 2022 04:20:55 +0800
Subject: [PATCH] tmp

---
 .../src/transformation/file/file.ts           |  2 +-
 .../src/index.ts                              | 33 +++++++++----------
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/packages/babel-core/src/transformation/file/file.ts b/packages/babel-core/src/transformation/file/file.ts
index cf687fb0b1..0a7b370f27 100644
--- 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-plugin-transform-computed-properties/src/index.ts b/packages/babel-plugin-transform-computed-properties/src/index.ts
index 0f911ecc9e..8d23d849c9 100644
--- a/packages/babel-plugin-transform-computed-properties/src/index.ts
+++ b/packages/babel-plugin-transform-computed-properties/src/index.ts
@@ -17,6 +17,15 @@ type PropertyInfo = {
   state: PluginPass;
 };
 
+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);
 
@@ -40,24 +49,14 @@ export default declare((api, options: Options) => {
     } else {
       // Fallback for @babel/helpers <= 7.20.6, manually add helper function
       const file = state.file;
-      helper = file.declarations["defineAccessor"];
+      helper = file.get("fallbackDefineAccessorHelper");
       if (!helper) {
-        helper = file.declarations["defineAccessor"] =
-          file.scope.generateUidIdentifier("defineAccessor");
-
-        const helperDeclaration = template.statement.ast`
-          function ${helper}(type, obj, key, fn) {
-            var desc = { configurable: true, enumerable: true };
-            desc[type] = fn;
-            return Object.defineProperty(obj, key, desc);
-          }      
-        `;
-        const helperPath = file.path.unshiftContainer(
-          "body",
-          helperDeclaration,
-        )[0];
-        // TODO: NodePath#unshiftContainer should automatically register new bindings.
-        file.scope.registerDeclaration(helperPath);
+        const id = file.scope.generateUidIdentifier("defineAccessor");
+        file.scope.push({
+          id,
+          init: DefineAccessorHelper,
+        });
+        file.set("fallbackDefineAccessorHelper", (helper = id));
       }
       helper = t.cloneNode(helper);
     }
-- 
2.38.1.windows.1

It seems better that way.

[SuperSodaSea]

Thanks @liuxingbaoyu!