Potential issues with conditional metadata and mutability

[jridgewell]

Given the following case's class structure:

// addMetadata doesn't do anything, I just want the @@metadata added
@addMetadata
class SuperClass {}

// Notice SubClass doesn't have a decorator, so it doesn't have @@metadata
class SubClass extends SuperClass {}

Because of the conditional semantics, the SubClass will not have the @@metadata object. And because the subclass's constructor's __proto__ will be set to SuperClass, accessing SubClass[@@metadata] will actually return SuperClass[@@metadata].

This can cause some potential bugs if the metadata of the class is mutated after the fact:

function doWorkWithMetadata(klass) {
  const metadata = klass[Symbol.metadata];
  if (metadata.__workDone) return;

  // do some work on metadata
  console.log(klass);

  metadata.__workDone = true;
}

doWorkWithMetadata(SubClass);
doWorkWithMetadata(SuperClass);

This will only "do work" on the subclass. Because the mutation will affect both the subclass and superclass, the superclass is skipped.

[ljharb]

Is that a bug, or is that actually correct behavior because the superclass would presumably benefit from that work being done and thus correctly indicate it was done?

[bakkot]

It's definitely a bug in at least some cases - information derived from the subclass may not be true of the superclass.

[hax]

How modifying a class metadata causing all "sibling" subclasses metadata changed would be "correct behavior"?

[hax]

I suppose conditional creating metadata object is mainly for perf, but consider this footgun I feel we should always create metadata for subclasses. I guess engines could optimize such thing, only really create it when first access or even when first try to mutate it.

[pzuraq]

Typically state is not tied directly to the metadata object, it's tied to the instance of the class. For instance, something like MobX might track which properties are reactive and store state for them, but that state would either be keyed off the instance or in the case of static properties off the class itself.

I don't think conditional metadata will be a common footgun because of this, and when encountered it's trivial to work around. I feel like mutating metadata at all after definition is like mutating class prototypes in general, in that it should be avoided unless you really know what you're doing, but I can see why someone might reach for it if they are new to decorators and metadata.

[endel]

Hi there, apologies if this is not the correct place for this discussion...

I'm not sure if this is an issue with the spec, or with core-js implementation of Symbol.metadata:

Question: Is it correct that constructor[Symbol.metadata] should be read-only?

import "core-js";

class MyClass {}
MyClass[Symbol.metadata] = {}; // Uncaught TypeError: Cannot assign to read only property 'Symbol(Symbol.metadata)' of function 'class (...)

I've created this repo for reference & including a workaround which I'm not sure it's against the standards... https://github.com/endel/core-js-symbol-metadata-issue

(On my encoder/decoder library (@colyseus/schema) I need to have control over the inheritance tree of the metadata (#18 ??). Thus, I need to be able to re-assign constructor[Symbol.metadata] to a different value. (see sources) -- Right now I'm still using legacy decorators, but already using Symbol.metadata for compatibility with "new" decorators in the future)

---

Really appreciate any clarification and/or suggestions if you have any! Thank you!