Using non-hoistable top-level code can cause problems with circular dependencies. For example (disclaimer: I didn't read the transform yet so this output does not match it, but it shows the problem):

// a.js
import "./b.js";

var getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;

function _applyDecorators(Class, decs) {
  getOwnPropertyDescriptor(Class, "foo");
}

export function getClass() {
  class _Class {};
  let Class = _applyDecorators(_Class, []);
  return Class;
}

// b.js
import { getClass } "./a.js";

getClass();

When running node a.js, getOwnPropertyDescriptor(Class, "foo"); will be run before getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor so it will throw that getOwnPropertyDescriptor is not a function.

Since Babel assumes that built-in functions behave as in the spec and are not modified, we don't need to extract all these Object.* and Array.* methods. For the remaining variables, we can just inline them (with comments like /* CONSTRUCTOR */ 0 to explain what the inlined value is).

We can probably get a slightly smaller output by doing

    ({
      0: initA,
      1: callB,
      2: initC,
      3: getC,
      4: setC,
      5: initInstance,
      6: Class,
      7: initClass,
    } = applyDecs(
      this,
      elementDecs,
      [dec]
    ));

Should this code log true or false?

function x() {}

function replace() {
  return x;
}

class C {
  @replace
  static #m() {}
  
  static getM() {
    return this.#m;
  }
}

console.log(C.getM() === x);

if it should log true, then this proxy does not work correctly. We might decide that it's ok and it's a known limitation, but we could make it work by converting #m to a field and replacing obj.#m = val assignments with obj, val, _readOnlyError("#m") calls (_readOnlyError is an existing helper).

This code should return true, but it definitely is not a common use case so it would be alright if we did not support this. I think the larger issue is that it seems private fields cannot be accessed prior to initialization, so for instance:

let getA;

class Foo {
  #a = ((this.#b = (v) => v), this.#b(123));

  #b;

  getA() {
    return this.#a;
  }
}

(new Foo()).getA()

This code does not work. With a true private method, it would work (tested in Chrome at least), and I think this is generally a more common pattern so I think we'll still want to do the proxy method here.

Uhm right, private methods would need to be "hoisted" before all the private fields when converting them.

hmm, yeah I think we can do that and that should work actually, will update

Nit: We can rename this to 2021-12, since it's when it was last presented (without @init).

Is this deleted property somewhere observable? i.e. can decorators access _Class before it's deleted?

We might consider storing it as a private method (which then doesn't need to be deleted), and pass an additional array of "private accessors" to _applyDecorators, like

applyDecs(
    this,
    elementDecs,
    classDecs,
    [o => o.#original_b]
);

If we end up using the "private method to fields" design suggested in the above comment, then we only need to do

static #b = function () {
  console.log('foo');
}

and then pass it like

applyDecs(
    this,
    elementDecs,
    classDecs,
    [this.#b]
);

So I came up with this before I realized that we could use static blocks (and in fact needed to in order to match the spec), and I just tested it out, we can actually refer to private names in static blocks. So we can fully extract private methods to the static block and avoid the delete in these cases 😄 I'll think about the best way to do this and update the code of applyDecs accordingly, no need to add private class fields.

Also here, if possible I'd prefer to pass them in a new last param of applyDecs rather than deleting.

Can you copy all these tests to also see how the transform behaves when only compiling decorators?

Edge cases: A member expression can also appears in a UpdateExpression or a destructuring assignment target:

this.#x++;
[...this.#x] = [];
for (this.#x of []);

hmm, in those cases I don't believe that the readOnlyError helper will really work, it'll be a syntax error. Should we just throw a compiler error if we find any assignments to the method? This may also be a capability we add in the future (see: tc39/proposal-decorators#438) so we could throw an error for now, and in the future if we end up supporting setting to decorated methods just remove that error.

We already handle that case poorly (https://babeljs.io/repl#?browsers=chrome%2080&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=MYGwhgzhAECC0G8BQ1oGIAeAKAlIgvitALYCmALgBYD2AJrokagNoB07VAlhK5gLrQAvNGZ8A3EUKEkAO1IB3OLlZkqdXEA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env&prettier=false&targets=&version=7.16.6&externalPlugins=&assumptions=%7B%7D is wrong), so a compile-time error is fine for now.

"unique to the given class" might not be enough. It must be unique in the given class and in all the class enclosing the current one. For example:

class A {
  #A = 1;
  static B = class B extends A {
    accessor a = 2;
    
    getA() {
      return this.#A;
    }
}

new A.B().getA();

we don't want to transform it to

class A {
  #A = 1;
  static B = class B extends A {
    #A = 2;
    get a() { return this.#A }
    set a(v) { this.#A = v }
    
    getA() {
      return this.#A;
    }
}

new A.B().getA();

otherwise it returns 2 rather than 1.

Now that this PR is mostly finished, could you add new commits without squashing 🙏 It makes it easier to see what changed since the last review.

Ok, updated the logic to remove the public UID part because we no longer use that, and now it takes in all referenced PrivateNames within the class body, so it should be sufficiently unique. If a duplicate key does exist within a parent, I don't think it should matter in the child unless the child references it, correct me if I'm wrong there.

Now that this PR is mostly finished, could you add new commits without squashing 🙏 It makes it easier to see what changed since the last review.

No prob!

Can you edit

We could also allow version: "legacy", but throw if both version and legacy are set. Then in Babel 8 we can remove legacy: true, to reduce the number of options.

This isn't true anymore, since we only use it for private fields?

Nit, but we could compact this code a bit by doing

let reifiedId;
do {
  reifiedId = String.fromCharCode(...currentPrivateId);
  incrementId(currentPrivateId);
} while (privateNames.has(reifiedId));

and currentPrivateId starts from [] rather than [charCodes.uppercaseA].

Is it correct that this uses Foo instead of _Foo?

So technically I don't think so, it should be using _Foo. However, I think that it's not really possible to do this, particularly with static private fields. We would have to extract all of the static fields and assign them to _Foo instead, but static private fields need to be defined within a class body to be valid.

I think there's a few options here:

1. Keep it as is, and note this as a limitation of the transform. The biggest meaningful difference would be that static public fields would not be own properties of the class, which I don't think is very common behavior to rely on.
2. Extract all static public fields, leave static private fields, and disregard ordering. I think that classes tend to rely on field initialization order more often than own-ness, so I don't think this is the best option.
3. Extract all static public fields, but assign them within static blocks in between private field assignments. Probably the most accurate transform, but also the most work.

We don't need ...args if the class does not have superClass , right?

We need to update the example here because the method implementation is now moved to the static block so x is no longer required.