Should public metadata objects inherit from null instead of Object.prototype?

[senocular]

As seen in the access section of the README, how one might iterate over public metadata would likely involve a for...in since it would include inherited keys.

for (let key in public) {
    instance[key] = this.lookup(public[key].injectionKey);
}

(More complete example below.)

However since public here includes Object.prototype in its inheritance hierarchy, this could likely include any other enumerable members possibly added there which would not be part of the actual metadata. This can be avoided if public were to inherit from null instead of Object.prototype.

As precedence import.meta also inherits from null instead of Object.prototype.

[ljharb]

Using for-in is a bad practice that many style guides prohibit. The likely form of iteration is using Object.keys/entries/values/assign, which only deal with own properties.

The spec very rarely produces null objects - and usually only when arbitrary string keys could conflict.

[zloirock]

In the spec, not so many cases where objects a used as key-value stores. In the modern JS it's there are even fewer of them. Offhand:

• Object.getOwnPropertyDescriptors, Object.fromEntries inherit Object.prototype
• RegExp#proups, modules namespaces, Array#@@unscopables and discussed recently Array#groupBy does not inherit Object.prototype

So null-proto objects win.

[ljharb]

"win" by counting isn't the way it would be decided.

[zloirock]

I don't care will it inherit Object.prototype or not, it's a counterargument against your "The spec very rarely produces null objects".

[ljharb]

The spec rarely produces (non-primordial) objects at all, so my statement would remain true even if 99% of those cases had a null prototype.

However, you forgot quite a few that inherit from Object.prototype:

• Object.getOwnPropertyDescriptor (singular)
• IterationResult objects (and async iteration result objects)
• unscopables is a builtin so i'm not sure it counts as an object produced by builtins (or else, Math, JSON, Reflect, Atomics, and globalThis would all count)
• arguments objects
• Proxy.revocable
• Promise.allSettled settlement objects

[zloirock]

They are not key->value stores or have a known set of possible keys, some of them are legacy from the ancient era, so it makes no sense to take them into account.

[ljharb]

Indeed, which is why i said "usually only when arbitrary string keys could conflict". Object.fromEntries isn't legacy, and at this point ES6 is pretty ancient :-)

Since metadata keys are always only Symbols, however, and since there are no Symbols on Object.prototype, there is zero possibility of accidental collision, so there is virtually no benefit from choosing a null prototype here.

[zloirock]

Seems you are inattentive, I mentioned Object.fromEntries and many cases with null prototype.

Symbols also are keys and someone can set symbols to Object.prototype just to break or get access to something.

[ljharb]

Also true. But, since nobody should be using for-in ever anyways for that exact reason, it remains a non-problem.

[zloirock]

It's not related to iteration, it's related just to access without helpers from this proposal and checking own property in any specific logic.

[ljharb]

It's a symbol. The only way the problem could happen is if all of the following happen:

• you fail to unconditionally set metadata under every symbol you use
• you leak the symbol, if it's private metadata (not 100% sure about this one; maybe they're all discoverable ¯\_(ツ)_/¯ )
• someone intentionally locates your symbol and uses it to mutate Object.prototype

Since the first one (and possibly the second) is utterly under your control, and the third one is exceedingly unlikely, I don't think it's a particularly pressing problem.

[zloirock]

Now it's symbols at first for preventing conflicts. In many cases, will be used Symbol.for or symbols will be public as a part of tools API. In some other cases will be possible to detect them. But not all parts of applications or instances are public, adding accessors to Object.prototype by those symbols will allow getting access to data from private instances.

[ljharb]

Again, only if you fail to always unconditionally set something. The setMetadata API uses Define, not Set, so Object.prototype accessors would not be triggered in this case.

[zloirock]

it's related just to access without helpers from this proposal and checking own property in any specific logic

[ljharb]

Right - but there is no access that would come before using setMetadata, which if done unconditionally, 100% prevents any problem, and since doing so would be a best practice anyways (so metadata has a consistent shape), it remains a non-issue.

[senocular]

Since metadata keys are always only Symbols, however, and since there are no Symbols on Object.prototype, there is zero possibility of accidental collision, so there is virtually no benefit from choosing a null prototype here.

This isn't about symbol keys on the Symbol.metadata object, its about the string keys in, and inheritance of o[Symbol.metadata][yourWhateverSymbol].public.

But, since nobody should be using for-in ever...

The for...in code snippet is taken straight out of the proposal. If that is not the intended usage, an alternative approach should be used.

[ljharb]

@senocular i would expect those string keys to always be unconditionally present, so their presence on Object.prototype would be irrelevant.

I totally agree that the readme examples should avoid using for..in.

[senocular]

I'm not sure if I'm conveying my concern correctly. Let me provide an example:

const TRACK = Symbol();
function track(val, { setMetadata, name }) {
 setMetadata(TRACK, name);
}
 
class SuperClass {
  @track superField = 'super'
}
class MyClass extends SuperClass {
  @track myField = 'my';
}

Object.prototype.thirdPartyExtension = function () {
  return 'thirdPartyExtension'
}

const instance = new MyClass
for (let tracked in instance[Symbol.metadata][TRACK].public) {
  console.log('for...in:', tracked)
}
/* Output:
for...in: myField
for...in: superField
for...in: thirdPartyExtension
*/

// vs

for (let tracked of Object.values(instance[Symbol.metadata][TRACK].public)) {
  console.log('for...of keys():', tracked)
}
/* Output:
for...of keys(): myField
*/

The problem being, for...in will get you all the inherited metadata, but it would also pick up anything enumerable in Object.prototype. Using for...of with something like Object.keys() will only get you own keys and not pick up the inherited metadata.

I actually ran this through https://javascriptdecorators.org/ and noticed they're actually using Object.create(null) for public now, though I thought looking at the spec OrdinaryObjectCreate(%Object.prototype%) is being used (assuming I'm looking in the correct place). - This has since been corrected

[pabloalmunia]

Umm, the experimental transpiler was develop before the formal specification and perhaps we assumed the Object.create(null) when the metadata structure was different and the .constructor was a problem. It is true that the specification indicates OrdinaryObjectCreate(%Object.prototype%) so our implementation is wrong.

[ljharb]

@senocular ah, i see what you're saying. in that case, if you added a if (typeof tracked === 'symbol') guard, you'd avoid the problem with string keys.

It is a good point that the design of metadata relying on inheritance means that there's no non-bad way to enumerate all the metadata, regardless of the [[Prototype]] of the objects.

[senocular]

in that case, if you added a if (typeof tracked === 'symbol') guard, you'd avoid the problem with string keys.

Then you wouldn't get any of the metadata since setMetadata stores them all as strings given they're string keys in the class ;)

[pabloalmunia]

It is true that the specification indicates OrdinaryObjectCreate(%Object.prototype%) so our implementation is wrong.

We have fixed this bug and publish a new version of https://javascriptdecorators.org/

[pzuraq]

Reviewing old issues at the moment, this was something that I believe was incorrect about the spec. For this metadata API, for..in would be considered the best way to iterate the inherited metadata, and so we want to use a null object at the top so users don't also iterate over Object.prototype values.

To be clear, I think that's the best choice and design for this API, which was designed to be opinionated and easily used for most metadata use cases. If using a null object or for..in are too unusual for the spec, then maybe a simpler API as discussed in #424 will make more sense.

[pzuraq]

After further discussion, we decided that this object should not inherit from null, since it would be a very unusual API in the spec if we did so. for..in should not generally be used to iterate this object, instead a utility that gets the inherited keys should be used instead. I'm planning on writing up a stage 0 proposal for such a utility to be added to the language.

As such, I'm going to close this.