Replace underscore properties with symbols

[mjethani]

Background

The ABP WebExt codebase uses "underscore properties" (property names that begin with an underscore) for encapsulation. While this is fine, there is now a better way to implement encapsulation with ES2015: ​symbols.

For example, we have the following code in ext/background.js:

let PageMap = ext.PageMap = function()
{
  this._map = new Map();
};

We could replace it with the following:

const map = Symbol();

let PageMap = ext.PageMap = function()
{
  this[map] = new Map();
};

Every instance of ext.PageMap would then have an enumerable symbol property that would generally stay hidden from the outside world (note: you could still go out of your way to access it using Object.getOwnPropertySymbols).

What to change

Replace ._foo with [foo], where foo is defined to be a unique symbol.

[kzar]

This sound sensible to me, assuming browser support is there and everyone agrees.

If so we should also take care to update the coding style rules and (if possible) ESLint configuration. Please could you file blocking issue(s) if so?

Note: I've noticed places in our codebase where we follow the "underscore prefix means private" convention needlessly for "static" variables / methods. We could have instead placed them outside of the Object in question and avoided exporting them! So we should be careful to not blindly switch to using Symbols for those cases, and instead simply move the private property out of the Object._

[agiammarchi]

My 2cents.

symbol property that would generally stay hidden from the outside world

There is usually a misleading feeling about Symbols and security, even simple Object.assign will move symbols around because enumerable by default, and same goes forObject.getOwnPropertyDescriptors(source), as well as the mentioned Object. getOwnPropertySymbols one.

If the goal is to have private variables there's only a way in JS to do that: WeakMap.

const wm = new WeakMap;

const PageMap = ext.PageMap = class PageMap {
  constructor() {
    wm.set(this, {map: new Map});
  }

  // basic private usage example
  method(key, ...rest) {
    const map = wm.get(this).map;
    if (rest.length === 0)
      return map.get(key);
    else
      map.set(key, rest[0]);
  }
};

If the goal is to avoid copies of the "secret" key around, then we could keep a semantic underscore as a prefix and use Object.defineProperty(this, '_prop', {writable: true, value: new Map}) in the constructor or, if we prefer symbols, use Object.defineProperty(this, mapsmb, {writable: true, value: new Map}) at least Object.assign won't easily copy properties around.

[mjethani]

Replying to agiammarchi:

My 2cents.

symbol property that would generally stay hidden from the outside world

There is usually a misleading feeling about Symbols and security, even simple Object.assign will move symbols around because enumerable by default, and same goes forObject.getOwnPropertyDescriptors(source), as well as the mentioned Object. getOwnPropertySymbols one.

One thing we have to keep in mind is that we would be doing this for encapsulation, not security. In C++ too you can access private members of a class using pointers, this doesn't mean the private keyword is useless. Before C++ came along you could do private members of structs in C, but it was tedious; C++ merely formalized the concept and helped turn it into a common practice.

If the goal is to have private variables there's only a way in JS to do that: WeakMap.

[snip]

This is awesome, but I wonder about the performance.

For what it's worth I do think that these properties should not be enumerable. We could perhaps have a utility function like so:

function definePrivateProperty(object, property, value,
                               {enumerable = false, writable = true, configurable = false} = {})
{
  Object.defineProperty(object, property, {value, enumerable, writable, configurable});
}

[agiammarchi]

I wonder about the performance

it depends how many instances are needed or how frequently these properties are accessed.

For example, accessing properties directly can be optimized by engines, accessing via square brackets might not.

this._prop; // always fast
this[prop]; // not always fast

In that case having a property assigned as non enumerable is better than using symbols.

About weakmap, setting or getting values is very fast. I have various UI benchmarks based on thousand objects that use WeakMap behind the scene but it's sure that once you assign an own property to an object, as long as you don't ever delete it (configurable = false helps with your proposal) it's the fastest way.

Then if performance are a concern, and we don't need to create a map per each instance, we could go down the lazy property/symbol assignment path:

const map = Symbol();

class WithMap {
  get [map]() {
    return Object.defineProperty(this, map, {value: new Map})[map];
  }
  get _map() {
    return Object.defineProperty(this, '_map', {value: new Map})._map;
  }
}

const m1 = new WithMap;

m1[map].set('test', value);
m1._map.set('test', value);

I'd be curious to actually see some little benchmark around these patterns, it'd be cool.

We could perhaps have a utility function like so:

I like the idea, but I'd remove private from the name 'cause it's not really private, IMO.

[sergz]

I'm still undecided, so I would like to merely share some additional thoughts to what already said for your information so far.

Can we firstly define what problems we are experiencing (perhaps even project-specific) what will be solved by every proposed approach, what it will cost us, and afterwards how urgent it is? I would like to remind about ​KISS principle.

For example, the approach with symbols can look attractive because

• it is consistent with using Symbol.iterator and other well known symbols
• it can improve robustness if there are mixins. BTW, is the latter our case?

What it costs, beside IMO more complicated code, for instance in adblockpluscore we are experiencing some difficulties (e.g. see hub tickets 5558 and #5735), which are very essential for us, and therefore we are looking into the ways for improvements, including using WebAssembly, emscripten, another data structures for filter classes and so on. In that regard I would say that particularly for adblockpluscore before switching the approach one should measure the impact on the performance and memory (who knows maybe switching to symbols can even optimize particular places), please pay attention that for other projects it can be not an issue. In addition to that, perhaps in the long term it will not be essential for adblockpluscore either.

[agiammarchi]

I would like to remind about ​KISS principle.

me too, hence I believe you already have the KISS in place, no need for Symbols at all ;-)

About this:

it is consistent with using Symbol.iterator and other well known symbols

it has nothing to do with Symbol.iterator. That is a special symbol used indirectly via for/of or other mechanism. Symbols used the way you propsed here are not special at all.

it can improve robustness if there are mixins

symbols solve name clashing but mixins through classes usually don't solve much and internally you want to use scope / private references more than symbols, unless I have misunderstood the use case.

Back to the topic:

Can we firstly define what problems we are experiencing

I absolutely agree and I have the tendency to jump into discussions without asking that first. This ticket description though talks about encapsulation only, but now there are performance concerns.

I think the perf issue is in the object VS map, but here it's about how to assign that map and if you have many instances with a mandatory map you should benchmark what's faster to set and retrieve.

I'll try to write a little bench because those I've found are covering different cases we don't care.

[agiammarchi]

So, I went ahead and created 2 benchmarks regarding what's been discussed in here.

Creating a property

The first benchmark is about creating a new Map properties through the following procedures:

• right away as own enumerable "_map" property
• right away as own enumerable map symbol
• through Object.defineProperty as own frozen, non enumerable, "_map" property
• through Object.defineProperty as own frozen, non enumerable, map symbol
• as weak reference with an object holding private properties such "map"

The benchmark is here:
​https://jsperf.com/create-prop-symbol-hidden-map

What I could observe is that property and symbol seems to have very similar performance while on creation time, the WeakMap way seems to be consistently slower (still reasonably performant though)

Accessing a property

Once the property is set, the second benchmark tests performance in accessing such property multiple times once created.

The procedures to do so are described in here:

• through direct obj._map property, both enumerable and frozen property
• trough dynamic obj[map] symbol, both enumerable and frozen symbol
• through runtime wm.get(obj).map weak relation

The benchmark is here:
​https://jsperf.com/retrieve-prop-symbol-hidden-map

What I could observe is that direct enumerable obj._map is faster than anything else while every other has very similar performance, including the WeakMap retrieval.

Personal Conclusion

The bottle neck we have in #5735 is about using the created map, not how the map is attached to the instance itself.

Accordingly, if raw performance in both setting and accessing such map is a concern, these benchmarks seems to suggest current way is already the best way to go.

Since every alternative has very similar performance or, to better phrase it, non concerning performance, if the goal is to ensure private properties, keep mixins ability, and have more robust code that won't expose symbols to the outer world, I'd say WeakMap would be my pick.

Worth saying I've used WeakMap for quite some time now, and it's never been the bottleneck of anything I've ever created (but it could in some case, I've just never witnessed those cases).

I hope this was anyhow helpful. As last thing I'd like to say I've tested only Chrome, maybe Firefox and Edge results will differ a lot so that we can have a better scenario to make the right choice.

Best Regards

[agiammarchi]

FYI: after few tests it looks like WeakMap is the slowest across all browsers while direct obj._map the fastest. There is a curious case for the hidden/frozen property, it looks like both Edge and Firefox are capable to optimise it and produce fastest read/access results, sometimes faster than direct obj._map property.

[mjethani]

Would it make a difference if we used a function instead of a class in the tests?

i.e. Function MapProperty() { ... } and MapProperty.prototype = { ... }; instead of class MapProperty() { constructor() { ... } ... }

I'm wondering about this because we do use the former in our code.

(Whether to switch to ES6 classes would be a separate question and something we should discuss in a separate Trac issue).

[sergz]

it is consistent with using Symbol.iterator and other well known symbols

it has nothing to do with Symbol.iterator. That is a special symbol used indirectly via for/of or other mechanism. Symbols used the way you propsed here are not special at all.

Perhaps it's a very bad example, sorry. I mentioned it because Symbol.iterator is a symbol too, and usually it's not supposed that one will work directly with a property identified by it, what is the same as with using symbols for private properties. One can get access to the properties of both types (accessible by custom or special symbols) but it's some sort of a hidden part of an object. IMO this is a basic concept and the functionality like for/of is based on this concept, namely there is a convention that a JS engine, which has access to everything anyway, can obtain the iterable interface what can be configured by a user using a special symbol available as Symbol.iterator.

it can improve robustness if there are mixins

symbols solve name clashing but mixins through classes usually don't solve much and internally you want to use scope / private references more than symbols, unless I have misunderstood the use case.

It seems all right, mixins allow to extend an interface of an object, no matter how exactly it's implemented there is always a probability of name clashing and having unique property IDs (what symbols are for) helps to reduce that probability.

Back to the topic:

Can we firstly define what problems we are experiencing

I absolutely agree and I have the tendency to jump into discussions without asking that first. This ticket description though talks about encapsulation only, but now there are performance concerns.

The title says "Replace underscore properties with symbols", it has the aim (encapsulation), concerns, consequent actions (e.g. comment:3), can discover other ideas etc, so it's normal.

I think the perf issue is in the object VS map, but here it's about how to assign that map and if you have many instances with a mandatory map you should benchmark what's faster to set and retrieve.

The performance and memory issues are not only in the object VS map, that particular case is merely the tightening nuts. Assigning of properties plays indeed a huge role in both memory usage and performance in JS engines and recently we got a really pressing requirements on performance and memory usage. I just wanted to say that before we make a convention we should carry the proper profiling and take the results into account, perhaps it will help a bit too or we should exclude certain modules. BTW, FYI while profiling our code and trying some micro optimizations like Object vs Map I noticed that sometimes what is supposed to be helpful does not really help because of different reasons, a lot of additional processes are happening in JS engines.

[agiammarchi]

@mjethani

Would it make a difference if we used a function instead of a class in the tests?

the benchmark passes through the same code in all test cases, only the property assignment is different so I'd say no, it wouldn't matter much.

@sergz

I mentioned it because Symbol.iterator is a symbol too

sorry for insisting :-) but indeed it was a very bad example for the following reasons:

• by default, Symbol.iterator is non enumerable because it's used on class level, not assigned to instances. Array.prototype.propertyIsEnumerable(Symbol.iterator) is false indeed. Being on class level, it's never accidentally copied around.
• it's a well known symbol, hence it suffers clashing with mixins so it doesn't solve any mixin issues, it actually causes some

I agree using symbols to carry some mixin related info around might be useful but WeakMap are a better solution to clashing and security.

If performance is not the best though, then I agree symbols can be a good alternative, but I would consider setting them once as writable, non enumerable, and non configurable, as you suggested through the function.

I would also consider the lazy pattern if the property is not always needed/used, otherwise both performance and memory would be affected. The lazy pattern, something I might add to the benchmark, is slower only the first time it's accessed, then is as fast as any other direct/dynamic access.

a lot of additional processes are happening in JS engines

absolutely, which is why even my benchmark should be considered with caution: that's not real world code running, it's a very easy to optimize in engine procedure.

However, where engines are not too smart (cough, Edge, cough ..) you can better see where performance falls.

About Object vs Map though, it's well known and documented Chrome creates hidden classes every time an object is created with one or more properties attached.

If we have objects with O(N) different properties each object, then we'll have O(N) hidden classes. If we delete properties or change/attach these frequently at runtime, then we'll have O(N*ops) hidden classes and greedy GC operations.

If we use Map instead, there are no hidden classes created so while performance in both set and get might be slightly slower on average, the memory usage is better, no hidden classes are created, the profiler should look happy.

At least these would be my expectations accordingly with how V8 works. Then again, maybe the engine trashes maps and uses objects with hidden classes behind the scene so it's very hard to find out the best solution.

Hopefully micro benchmarks can help a little though.

[agiammarchi]

FYI I have added the lazy pattern that reveals performance during instance creation and similar perf on setting and repeatedly getting the value, either as prop or as symbol.

[mjethani]

When I filed this issue I was thinking about readability and maintainability, i.e. making code easier to reason about and harder to shoot yourself in the foot with. I did not consider performance. If performance is only +/-10% or so then I wouldn't worry about it too much personally at least for WebExt (but perhaps it matters for core).

The WeakMap-based approach for encapsulation is harder to maintain IMHO, so unless there's a good reason to go with it we should avoid it.

[agiammarchi]

The WeakMap-based approach for encapsulation is harder to maintain

I think that's subjective, but my point was rather about the goal of the ticket.

One of the reasons for symbols was that "symbol property would generally stay hidden from the outside world" so my replies were based on that assumption, which is indeed a footgun for developers believing symbols are like private properties.

Symbols are instead easily exposed, if directly assigned, due Object.assign(target, source) behavior, or exposed, regardless the way these are assigned, via Reflect.ownKeys, Object.getOwnPropertyDescriptors, and Object.getOwnPropertySymbols.

So, as long as everyone is aware of that, feel free to go ahead and apologies for me chiming in :-)

[mjethani]

Replying to agiammarchi:

One of the reasons for symbols was that "symbol property would generally stay hidden from the outside world" so my replies were based on that assumption, which is indeed a footgun for developers believing symbols are like private properties.

Would you say that, as far as "privateness" is concerned, symbols are an improvement over underscore properties?

I would love to have actual private properties in JS, but to try to implement that using a WeakMap doesn't make sense for every little class (e.g. see the ext.PageMap class itself, it's really small, has only one internal property and a handful of methods). Maybe there are individual classes where it makes sense to do this, but certainly not for every class. Switching to symbols, on the other hand, is almost a find-and-replace operation in the source.

[agiammarchi]

Would you say that, as far as "privateness" is concerned, symbols are an improvement over underscore properties?

Considering Object.keys and for/in wouldn't expose them, yes, it's an improvement.

Yet if we use Object.assign it's easy to leak them by accident if directly assigned as enumerable.

using a WeakMap doesn't make sense for every little class

a pattern is a pattern, IMO. If you have the convention that privates are set once and retrieve via a WM it's not too ugly/difficult to maintain:

// one privates per scope/class
const pvt = privates.get(this);
// all properties there
pvt.value = 123;
pvt.other = 456;

If you want to use private methods though, the indirection becomes awkward so I'd prefer the classic scopedFunction.call(this) instead.

However, the WeakMap pattern I've suggested will quite inevitably be the de-facto standard once/if private fields land in ECMAScript, now at stage 1:
​https://tc39.github.io/proposal-private-fields/

There's no other way to simulate privates, so I'm pretty sure Babel will base its transformation on WeakMap (but at this point I'm just speculating though).

Worth mentioning even private fields won't solve the mixins clashing issue while a WeakMap approach will never see a clash because each reference is per mixin, not per instance/derived object.

Non enumrable symbols might also be a good alternative, if associated properties can be copied around via shallow clones.

Regards

[mjethani]

How about this:

/* superawesomebaseclass.js */

class SuperAwesomeBaseClass
{
  constructor(internal)
  {
    Object.defineProperty(this, internal, {
      enumerable: false,
      writable: false,
      configurable: false,
      value: Object.create(null)
    });
  }
}

/* pagemap.js */

const internal = Symbol();

class PageMap extends SuperAwesomeBaseClass
{
  constructor()
  {
    super(internal);

    this[internal].map = new Map();
  }

  getValue(page)
  {
    return this[internal].map.get(page.id);
  }

  setValue(page, value)
  {
    this[internal].map.set(page.id, value);
  }
}

/* mycode.js */

let pageMap = new PageMap();
pageMap.setValue({id: 123}, "Hello");
console.log(pageMap.getValue({id: 123}));

So:

1. this[internal] is non-enumerable, non-writable, non-configurable at the time of object construction
2. What SuperAwesomeBaseClass does is encapsulated in its implementation file
3. Every class that derives from SuperAwesomeBaseClass has a unique symbol called internal private to its scope (always called "internal" by convention though it could be called anything within its source file)
4. PageMap is modified to change this._map to this[internal].map and that's the only change

Can we modify SuperAwesomeBaseClass to use a WeakMap instead? I don't mind how the encapsulation is implemented as long as we have the encapsulation and is easy to use. e.g. in this case this[internal].map is not much different from this._map, it's also kind of more self-documenting than using an underscore in the property name. If we can do this with the WeakMap-based approach then I'm all for it.

[mjethani]

A minor improvement to this:

/* namespace.js */
function defineNamespace(object, namespace)
{
  Object.defineProperty(object, namespace, {
    enumerable: false,
    writable: false,
    configurable: false,
    value: Object.create(null)
  });
}

And then "namespaces" can be used for anything, using symbols or whatever:

/* pagemap.js */
const internal = Symbol();

class PageMap
{
  constructor()
  {
    defineNamespace(this, internal);

    this[internal].map = new Map();
  }

  ...
}

This is actually quite similar to ActionScript namespaces, which have a syntax like this::internal.map instead of this[internal].map but conceptually do the same thing.

[agiammarchi]

if I might suggest what I'd do:

/* namespace.js as module: exports defineNamespace */
const create = Object.create;
const defineProperty = Object.defineProperty;
const defineNamespace = (object, namespace) =>
  defineProperty(object, namespace, {
    value: create(null)
  });

Reasons:

• no global Object lookup and method access each time. Trap methods once for both security and performance
• no need to specify all defaults for new own properties. Namespaces are unique so, if already defined, will throw anyway
• no need to have a named function declaration, neither context nor arguments: just arrow is enough and const grants it cannot ever be redefined (in scope or once bundled)
• return the object in which the namespace is defined, symmetric with defineProperty, defineProperties, and/or setPrototypeOf

[agiammarchi]

P.S. for documentation sake, a similar pattern through the WeakMap:

/* pagemap.js */
const privates = new WeakMap();
const internal = $ => privates.get($) ||
                      (privates.set($, {__proto__: null}), internal($));

class PageMap
{
  constructor()
  {
    internal(this).map = new Map();
  }
}

Yet I do like the this[internal].map = new Map(); solution too.

Regards

[mjethani]

So just to complete this:

/* namespace.js */

function namespace()
{
  let map = new WeakMap();
  return $ => map.get($) || map.set($, {__proto__: null}) && map.get($);
}

/* pagemap.js */

const internal = namespace();

class PageMap
{
  constructor()
  {
    internal(this).map = new Map();
  }

  ...
}

[agiammarchi]

quick thoughts on these patterns: none of them plays well with inheritance. If a super.method() uses this[internal].value = 123, that value would be unreachable through the method that invoked the super because the internal symbol will be different too.

If inheritance wants to play nicer, the root class should have one internal, so we're actually back to lazy pattern.

const create = Object.create;
const defineProperty = Object.defineProperty;
const internalSymbol = Symbol();

class WithInternal {
  get internal() {
    const desc = {value: create(null)};
    return defineProperty(this, internalSymbol, desc)[internalSymbol];
  }
}

At that point every subclass would simply use this.internal.map = new Map when needed, super calls/setup work, etcetera.

Through the WeakMap it would be:

const create = Object.create;
const wm = new WeakMap;

class WithInternal {
  get internal() {
    return wm.get(this) || wm.set(this, {value: create(null)}) && wm.get(this);
  }
}

Regards

[mjethani]

@agiammarchi with the above approach the properties aren't really private or internal any more than they are right now, because the caller can simply do let wi = new WithInternal(); wi.internal.map = new Map();, right? The use of a symbol here is entirely superfluous, am I right?

We're trying to square a circle in a way. Without native support from JS we won't be able to get too far. So yeah, we can't do "protected" (C++) properties but we can do private (sort of).

[agiammarchi]

The use of a symbol here is entirely superfluous, am I right?

actually kinda yes, but I don't see alternatives: it either plays well with mixins (initial goal?) or it plays well with inheritance, but it loses, with my proposal, the "private" bit because there's no guard on who's invoking where.

ignore my last examples, it's just something to keep in mind if inherited "privates" are needed/used

[mjethani]

Alright, I think I've figured out a decent solution for the private methods as well.

We have code like this:

const internal = Symbol();

let PageMap = function()
{
  defineNamespace(this, internal);

  this[internal].map = new Map();
};

PageMap.prototype = {
  _deletePageById(pageId)
  {
    this[internal].map.delete(pageId);
  },
  delete(page)
  {
    this._deletePageById(page.id);
  }
};

So PageMap.prototype._deletePageById is supposed to be private, but it must also be able to use this, which makes it difficult.

But we can do this:

PageMap.prototype = {
  [internal]: {
    deletePageById(pageId)
    {
      this[internal].map.delete(pageId);
    }
  },
  delete(page)
  {
    this[internal].deletePageById(page.id);
  }
};

i.e. the prototype has its own [internal] property, which is also magically accessible via this[internal].

For this to work, the defineNamespace function must look like this:

const {create, defineProperty, getPrototypeOf, keys} = Object;

const {hasOwnProperty} = Object.prototype;
const {bind} = Function.prototype;

function defineNamespace(object, namespace)
{
  let prototype = null;

  // If the object has a prototype and the prototype has the same namespace,
  // copy over all the functions in that namespace to our prototype,
  // additionally binding them to the object.
  let objectPrototype = getPrototypeOf(object);
  if (objectPrototype && hasOwnProperty.call(objectPrototype, namespace))
  {
    prototype = create(null);

    let objectPrototypeNamespace = objectPrototype[namespace];
    for (let key of keys(objectPrototypeNamespace))
    {
      let value = objectPrototypeNamespace[key];
      if (typeof value == "function")
        prototype[key] = bind.call(value, object);
    }
  }

  return defineProperty(object, namespace, {value: create(prototype)});
}

[agiammarchi]

Just FYI this actually just happened: Chrome 66 (dev channel) is shipping behind a flag private fields.

If you'd like to play with it, I can spoil it reflects more the weakmap/symbol per class and no inheritance is ever involved so that this[internal] used in class A will never pass through this[internal] related to class B even if the accessor passes through an instance of B.

TL;DR all private fields apparently shadow each other per class. No clash ever.

[sebastian]

Creating a Map object for each instance defeats the purpose, both in terms of isolation and performance. If the Map object is assigned to the instance, the internal state is still exposed and you just add additional overhead compared to directly accessing properties of that object.

Manish made some attempt to address this by using a shared WeakMap. However, with a global WeakMap the private state still remains accessible by other code. (Also the PageMap class is a somewhat confusing example as by design it is a wrapper around a Map object.)

If you want to use a WeakMap for perfect isolation, you need one WeakMap object for each private state, shared across all instances, and isolate it in a shared closure with the object definition:

let SomeObject = (function()
{
  let privateState = new WeakMap();

  let SomeObject = function()
  {
    privateState.set(this, 42);
  };

  SomeObject.prototype = {
    getState()
    {
      return privateState.get(this);
    }
  };

  return SomeObject;
})();

For private methods there is no point in using a WeakMap but to achieve perfect isolation, just moving the function out of the object into a shared closure is sufficient:

let SomeObject = (function()
{
  function convertValue(obj, value)
  {
    return parseInt(value);
  }

  let SomeObject = function() {};

  SomeObject.prototype = {
    makeValue(value)
    {
      return convertValue(this, value);
    }
  };

  return SomeObject;
})();

Finally, Symbols merely prevent name clashes (e.g. when dealing with mixins, mocks and complex inheritance), but don't provide any deeper isolation than named properties.

All three techniques (i.e. hidden WeakMap objects, hidden functions and Symbols) increase the boilerplate and impair the code locality, while their advantages seem to be neglactable in (almost) all our cases.

[mjethani]

If nobody has any objections in the next couple of weeks, I would like to close this as rejected. At this point nobody in the team seems to think that this is a good idea. If there is interest in the future we can reopen it.

[kzar]

Fine by me, personally I'd rather just avoid putting something that's private inside the Object in the first place.

[agiammarchi]

FYI private fields landed not only in Chrome 66+, but also in Babel 7. Apparently (and imo a bit surprisingly) one WeakMap per field is the specs-compliant behavior so that the following input:

class A {
  #a = 1;
  #b = 2;
}

class B extends A {
  #b = 3;
}

class C {
  #a = 1;
  #c = 4;
}

produces the following output:

class A {
  constructor() {
    _a.set(this, 1);

    _b.set(this, 2);
  }

}

var _a = new WeakMap();

var _b = new WeakMap();

class B extends A {
  constructor(...args) {
    super(...args);

    _b2.set(this, 3);
  }

}

var _b2 = new WeakMap();

class C {
  constructor() {
    _a2.set(this, 1);

    _c.set(this, 4);
  }

}

var _a2 = new WeakMap();

var _c = new WeakMap();

I have no idea about performance impact, but maybe if we'd like to implement anything that resemble private properties, we should probably stick with the standard way and use, maybe for this case only, the @babel/plugin-proposal-class-properties transformer until all target browsers support the new syntax.

Private methods AFAIK have never been an issue (.call / .apply a scoped function, that's it), but as long term maintenance strategy, since the transpiled Babel 7 version seems to be 100% standard compliant, I wouldn't create anything in-house or, if performance is a concern, I would simply stick with the _ prefix convention.

Just my 2 cents.

[agiammarchi]

For documentation sake, I've created ​a little benchmark that tests the following cases after transpilation:

const bench = {
  privates: () => class PrivateFields {
    #a = 'a';
    #one = 1;
    toString() {
      return `${this.#a}${this.#one}`;
    }
  },
  prefixed: () => class Prefixed {
    constructor() {
      this._a = 'a';
      this._one = 1;
    }
    toString() {
      return `${this._a}${this._one}`;
    }
  },
  weakly: () => {
    const _self = new WeakMap;
    return class Weakly {
      constructor() {
        _self.set(this, {a: '1', one: 1});
      }
      toString() {
        const {a, one} = _self.get(this);
        return `${a}${one}`;
      }
    }
  }
};

The result is that this._property is around 3X faster than transpiled code, where latter one is also slightly slower than a single WeakMap per class, which preserves compatibility with standards, and yet is not super performant.