Cannot decorate undefined property with deep inheritance

[uqee]

Reproducible example:

Please, have a look at
CodeSandBox.

Copy of the code

import { action, computed, makeObservable, observable } from "mobx";

class A {
  public constructor() {
    makeObservable(this);
  }

  // @action aAction = () => "aAction";

  // @computed get aGetter() {
  //   return "aGetter";
  // }

  // aMethod() {
  //   return "aMethod";
  // }

  // @observable aObservable = "aObservable";

  // aProperty = "aProperty";
}

class B extends A {
  public constructor() {
    super();
    makeObservable(this);
  }

  // UNCOMMENT ME TO BREAK
  // @action bAction = () => "bAction";

  @computed get bGetter() {
    return "bGetter";
  }

  bMethod() {
    return "bMethod";
  }

  // UNCOMMENT ME TO BREAK
  // @observable bObservable = "bObservable";

  bProperty = "bProperty";
}

class C extends B {
  public constructor() {
    super();
    makeObservable(this);
  }

  // UNCOMMENT ME TO REPAIR
  // @action cAction = () => "cAction";

  @computed get cGetter() {
    return "cGetter";
  }

  // cMethod() {
  //   return "cMethod";
  // }

  // UNCOMMENT ME TO REPAIR
  // @observable cObservable = "cObservable";

  // cProperty = "cProperty";
}

const c: C = new C();
console.log("OK", c);

---

How to reproduce the issue:

1. Uncomment either one from

   // UNCOMMENT ME TO BREAK

   sections to see an error:

   Cannot decorate undefined property: ...

2. Then comment either one from

   // COMMENT ME TO REPAIR

   sections to remove the error.

Versions:

• mobx@6.0.4
• typescript@4.1.2, "useDefineForClassFields": true
• @babel/plugin-proposal-class-properties@7.12.1, "loose": false

[urugator]

You shouldn't call makeObservable if there are no decorators/annotation (class A). Otherwise possibly similar issue to #2593.
Subclassing currently isn't ideal, we are trying to come up with something less fragile.
cAction = action(() => "cAction") should be a future-proof solution for arrow functions that needs to be overridable by subclass.

[uqee]

If class A is not empty it fails anyway, I couldn't find any relation with its content, so commented it for brevity. Also it fails without any overridable properties or even actions.

Seems like the class C's @computed causes the trouble and adding any other decorator alongside it is a possible workaround. I would have investigated @computed decorators further which seem somehow flawed.

[urugator]

Also it fails without any overridable properties or even actions.

I've uncommented observbales/actions in B/C and commented out makeObservable in A and it doesn't throw in sandbox...
If I uncomment everything it also doesn't throw...
Whether everything is correctly converted to observable/computed/action I dunno...

[mweststrate]

As a work around, I think in principle it should work fine to not apply makeObservablein super classes when using decorators, and apply it in subclasses only. If subclassing is not guaranteed, a condition like if (this instanceof SuperClass) makeObservable(this)should work I'd expect

[uqee]

Good call, thanks, but didn't work either (tried with the example).

[jeremy-coleman]

What do you guys think about something like a mobx specific version of util/inherits designed for extending mobx decorated classes? I've found this is a huge foot gun and really hard to track down the source of the problem. From a user perspective, it would have the benefit of making it very obvious that a subclass's parent has mobx observable members. I'm ofc not a maintainer, but from a maintainers perspective, it might be easier to have a single point of origin to work on all class extending related issues

[urugator]

@jeremy-coleman Have you checked #2641? It restricts overriding only to actions defined on prototype and should throw in all other cases. However it's not entierely BC.
A BC solution is probably possible, but it would have almost the same limitations and no way to enforce them programatically, so user could still mess things up without knowing.

[jeremy-coleman]

I did , thank you for directing me there and your work on the pr. I have been thinking about it for a while, but still unsure of any conclusion.

[ahoisl]

As a work around, I think in principle it should work fine to not apply makeObservable in super classes when using decorators, and apply it in subclasses only. If subclassing is not guaranteed, a condition like if (this instanceof SuperClass) makeObservable(this)should work I'd expect

This worked for my use case, thank you!

Would still be great if #2641 was a remedy for this issue as well, because makeObservable should be called as close to the relevant props (in the class constructor) as possible. Right now, it's a bit error prone with a high chance to forget the call in inheriting classes

[mweststrate]

MobX 6.1.0 has been released with improved inheritance, which should address this issue.