Allow compiling #foo in obj without compiling private fields

[nicolo-ribaudo]

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

Currently we throw an error when compiling private brand checks without compiling private fields. This makes it impossible to add this plugin to preset-env, since it will throw an error in browsers with private fields support but without brand checks support.

This PR adds a new option to compile private brand checks without relying on the private fields transform: preset-env can then enable this option when necessary.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7fcf7c4:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

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

[JLHwung]

We can also place FooBrandCheck to a private static field, e.g.

class Foo {
  private static #_FooBrandCheck = new WeakSet();
  constructor() {
    Foo.#_FooBrandCheck.add(this);
  }

  get #foo() {}

  test(other) {
    return Foo.#_FooBrandCheck.has(other);
  }
}

so we avoid the edge cases when FooBrandCheck might be injected to an incorrect scope, e.g. when a class is in the param initializer.

// <-- FooBrandCheck will be injected to here
(x = class { #foo; test (other) { return #foo in other } }) => {}

[nicolo-ribaudo]

Ohh I love this idea

[JLHwung]

I wish class access were not stage-2. The class binding can be overwritten. Guess we may have to stick to temporary variable.

[JLHwung]

Q: Is _FooBrandCheck.has(other) equivalent to other instanceof Foo?

[nicolo-ribaudo]

No, it's different when other is a subclass of Foo

[JLHwung]

If Bar, a subclass of Foo, has super() in its constructor, does it imply #foo in bar is true where bar is an instance of Bar?

[nicolo-ribaudo]

Oh I was confusing this test with the private fields one. instanceof is different because it checks the prototype:

class Base {
  constructor() {
    return {};
  }
}

class Foo extends Base {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

new Foo() instanceof Foo; // false
Foo.check(new Foo); // true

class Foo {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

let foo = new Foo();
foo.__proto__ = {};
foo instanceof Foo; // false
Foo.check(foo); // true

class Foo {
  #foo;

  static check(obj) {
    return #foo in obj;
  }
}

let foo = Object.create(Foo.prototype);
foo instanceof Foo; // true
Foo.check(foo); // false

If Bar, a subclass of Foo, has super() in its constructor, does it imply #foo in bar is true where bar is an instance of Bar?

If by "bar is an instance of Bar" you mean bar instanceof Bar then no, there isn't an implication in any direction.

[jridgewell]

I think we're going to have ordering issues:

class Foo {
  #x = 1;
  y = #x in this;
}

assert.equal(new Foo().y, true);

[nicolo-ribaudo]

Fixed both the bugs!

[JLHwung]

can we default nativePrivateFields to !isRequire("proposal-private-methods", api.targets())?

[hzoo]

This isn't necessarily intended to be a public api right since we can just check targets? Or did we want to be able to specify that

[nicolo-ribaudo]

Ideally every plugin should automatically compile as few as possible based on the targets, but users should still be able to override it via options.

[jridgewell]

Would it be easier to rename the class, instead of all conflicting bindings?

[nicolo-ribaudo]

I think I'll leave this as-is: renaming a class is always observable (because it changes the .name), while renaming the other conflicts is often not observable (if we rename a variable). It can still be observable if the conflict is with a class/function, but at least it's not always.

[jridgewell]

Nit:

Suggested change

	classWeakSets,
	staticWeakSets,

[nicolo-ribaudo]

This is used for instance methods, but it's a single weakset for all the instance methods in the class.

[nicolo-ribaudo]

@JLHwung Rather than using targets, I realized that we can always use the new implementation (bf01056). All the tests in https://github.com/nicolo-ribaudo/babel/tree/private-in-to-native-private/packages/babel-plugin-proposal-private-property-in-object/test/fixtures/private ensure that we keep the old behavior when compiling private fields.

[JLHwung]

@nicolo-ribaudo Does the new implementation increase the output size when all class features are compiled? If so I would prefer to use it only when private elements are natively supported.

[nicolo-ribaudo]

When all the class features are compiled we match the old output, because it's handled by the class features helper enabled by one of the other plugins. You can see that bf01056 didn't change any test, except for removing an error.