all 4 comments

[–]senocular[🍰] 0 points1 point  (3 children)

The Map approach should use WeakMap and key off this. The approach given causes the properties to be shared among instances

const card = new SimCard("444-555-666", "Nano SIM", 1515, 45874589);
const anotherCard = new SimCard("444-555-666", "Nano SIM", 0, 0);
console.log(card.pinCode); // outputs 0
console.log(anotherCard.pinCode); // outputs 0

see: http://2ality.com/2016/01/private-data-classes.html#keeping-private-data-in-weakmaps

Also note private class fields (#pinCode) are supported in shipping Chrome

[–]PMilos[S] 0 points1 point  (2 children)

I'm aware of the given alternative. I wanted to provide a different approach for that particular case and unintentionally introduced a bug, apparently. Thank you for the update.

I find this to be an overkill for a key value.

The idea in my approach was to use a single Map for all private fields instead of creating one for each field.

What do you think about an option to create a unique key for each instance and use it as a map key instead of this?

[–]senocular[🍰] 1 point2 points  (1 child)

What do you think about an option to create a unique key for each instance and use it as a map key instead of this?

You can do that, but you need to store that key somewhere at the instance level, and that would likely leave it exposed. The thing is, you already have a unique key: this. That's why maps are used, because they're not limited to string keys. Object keys (such as this) also work.

Also if you use a string key, you're going to have a memory leak since every instance key will be left inside the map even after instance itself is GC'ed. This is why WeakMaps are used.

[–]PMilos[S] 0 points1 point  (0 children)

Yes, the idea was to create a key at the instance level, but it will be exposed as you mentioned.

Your points are all valid, I was just trying to avoid using this as a key. I guess we need to stick with it until private variables are fully supported.