Why makeAutoObservable does not support subclassing

[cncolder]

I have read the document https://mobx.js.org/subclassing.html, and I understand the makeAutoObservable does not support subclassing now.

But I'm not sure the reason. In fact, I try to comment out this line

mobx/packages/mobx/src/api/makeObservable.ts

Line 52 in 0945c26

die(`'makeAutoObservable' can only be used for classes that don't have a superclass`)

. It works as expects. 🤔

class BaseStore {
  data = {}
}

class Store extends BaseStore {
  constructor() {
    super();
    makeAutoObservable(this);
  }
  
  loading = false
}

[urugator]

The situation where the extended classes are abstract (can't be instantiated) - meaning you can call makeAutoObservable only once in the "subbest" class - is kinda supportable, the problem is if the extended class is eg React.Component - in such case makeAutoObservable should ignore all it's fields and prototype.
In other cases, you must be able to call makeAutoObservable multiple times on the same object - this is problematic, because we don't know which fields of the object were already annotated and which were not (this is mostly due to the ability to provide overrides option to makeAutoObservable or when you would combine it with makeObservable). It would also prevented some optimizations.
We don't have this issue with makeObservable, because user is the one providing the list of fields to annotate.
Note, there is no way of telling, which field belongs to which class - eg in your example we don't know that data field comes from BaseClass, similary we don't know from which constructor make(Auto)Observable is being called, therefore we had to impose some restrictions on subclassing and annotating. More on that here.

[urugator]

Is there a reason why let current = proto; shouldn't be let current = target;

It's probably wrong. As I said, I've never try to run the code above, it's only to demonstrate the idea. Do not copy-paste it to production code as is. I've changed the mentioned bit however.

[stephenh]

Ah yeah, @KashubaK you are right; our local makeSimpleAutoObservable had your suggested current = target fix as well, just as @urugator 's edit now does.

So we must have also fixed it back in the day, but apologies didn't think to post it back up. :-/

Thanks for updating @urugator !

[Kashuab]

Thanks guys for the responses :) Cheers!

[inoyakaigor]

The same story as @stephenh's. Made that fix local but forget mention it here

[inoyakaigor]

@LandonSchropp After a few years testing these code in production I finally made a package: mobx-store-inheritance

Folks FYI @stephenh @KashubaK @cncolder

[stephenh]

That's awesome! Thank you! Fwiw this seems like a common ask from users, i.e. I nearly opened a brand new discussion/issue asking the same question (just to ask specifically what makeAutoObservables sharp edges are with subclasses to verify whether we could avoid them and afaict we are/can). What do you think about addong your snippet to mobx with an appropriate name and docs of "only use this if you know what you are doing in these x scenarios"? Or if anything maybe a link in makeAutoObservables docs that "if you really want subclasses see this gist that you can copy/paste and be on your own". :-)

[urugator]

No, because this is not a suggested way to go, therefore I don't want to promote it in any way. Whatever you expose, some people will use it, twist it, misuse it, and in the end it will always come back to you, no matter the warnings.
Automatic behavior provides convinience for common/simple cases. If it can't support inheritance reliably, the responsbile thing to do is either not introducing automatic behavior in the first place (no longer an option) or to forbid cases that could lead to issues.

I am quite convinced that the cost of using makeObservable instead of makeAutoObservable (or even better getting rid of inheritance) for these ~10 classes , will be lower than the cost of introducing and maintaning any workaround.

[stephenh]

That's a depressing conclusion :-) but I understand its born from your real world experience, so I understand. Thanks!

[LandonSchropp]

@urugator Just to throw my 2¢ into the discussion, I'm not sure if I agree with your assessment. The problem is it's really easy to add properties and methods to your store and to forget to add them to makeObservable. This can lead to nasty bugs where components updates when another property's value changes, but doesn't if the missing property is changed. These types of issues can be very tricky to track down. Personally, I'd rather have things handled automatically.

[urugator]

This can lead to nasty bugs

Ok but as mentioned the alternative is a solution that:

• can lead to other bugs (when extending foreign classes, which is impossible to detect)
• supports only extending abstract classes (again impossible to detect)
• requires different (and weird) usage than rest of the API (makeObservable should be called in each constructor, while makeAutoObservable only in the last constructor)

Unless you're dealing with existing codebase and moving away from decorators, it's not like you're forced to use inheritence and therefore use makeObservable and therefore introduce nasty bugs. You can use composition or you can use decorators, or you don't have to use classes... There are different approches with different (hopefully reasonable) tradeoffs, you just have to desing the system with these in mind and pick the one providing best value for your project.

That being said I think it could be interesting to have an eslint plugin (because it can't be done runtime), which checks that makeObservable lists all fields defined on a class.

EDIT: For future readers, we do have the plugin now https://github.com/mobxjs/mobx/tree/main/packages/eslint-plugin-mobx

[ThaJay]

I'm getting the message:
[Error: [MobX] Extending an object with another observable (object) is not supported]
without extra information.

I'm doing maintenance on an app that had development frozen for 2 years, so it's probably a migration issue with new package versions. Unfortunately, there is no stack trace or other hint what / where exactly it goes wrong, this would be nice to have.

I do have some subclassing, but that's only a common constructor to fill the stores with data. It used to work. I'll grab the docs and see if I can solve it. If I encounter something weird I'll post it here. For now, thanks for being a rubber duck :)

[urugator]

You're probably passing an observable object as second arg to extendObservable somewhere. It expects plain object instead.