makeObservable throws in dev environment when used with annotations on decorated object

[ahoisl]

PR #3154 caused a breaking change in dev environments for us, now that it dies when calling makeObservable passing annotations on a target object with decorators. The change was IMHO intended to prevent users from not noticing that the annotations are used instead of the stored annotations from the decorators.

As this may help in avoiding issues like #3152 , this was a breaking change for our use case, where we call both makeObservable with annotations and without on a target. The use case is e.g. if you have a common interface implemented by multiple classes and you always want to use the same annotations. Or if you want to automatically make all fields of an object observable (using a helper function), but still have specific properties that you want to mark as computed.

Intended outcome:

Sample code:

interface MyCommonInterface {
    field1: number;
}

class MySpecialClass implements MyCommonInterface {
    field1 = 42;

    @computed
    get doubled(): number { return this.field * 2; }
}

makeObservable(target, { field1: observable });
makeObservable(target);

// no error

No error.

Actual outcome:

makeObservable throws error "makeObservable second arg must be nullish when using decorators. Mixing @decorator syntax with annotations is not supported."

How to reproduce the issue:

Run the above code in a dev environment. Works fine in production.

Versions

mobx >= v6.3.6

[ahoisl]

There are various fixes for this behavior that I can think of, you probably even find something better?

1. Revert the change
2. Add a new option in the passed CreateObservableOptions, e.g. "allowAnnotationDecoratorMixing"
3. Create a general option
4. just log a warning instead of dying

I'm willing to provide a PR for the fix you deem the best

[urugator]

Mixing annotations and decorators isn't supported and it's noted in docs since 6.1.0:
https://mobx.js.org/observable-state.html#limitations
It just didn't throw until #3154. It could have worked in simple cases, but not in general.
You can call makeObservable how many times you want on the same target, but you have to stick with either decorator or annotation syntax.

Could you show a specific example of your use case? Perhaps we could find a way to workaround it somehow, dunno.

[ahoisl]

First, thanks for you quick reply! I'll try to give a little more background info.

Mixing annotations and decorators isn't supported and it's noted in docs since 6.1.0:

Well, we're using mobx since version 5.0 and used that approach since we migrated to 6.0. Before 6.0 we were using decorators only, but needed to switch to use annotations for our use case.

Our use case:

• frontend & backendboth using typescript; SPA with react & heavily relying on mobx
• we have business classes that are shared between backend and frontend
• classes are used for validation, storage, and communication
• no mobx in backend; frontend needs to "add" mobx observability to these classes
• in frontend we inherit from base business class and extend for usage in viewstore
• we have a helper function that mobx annotates all fields for the base class
• additional fields or helper properties in the frontend are then marked with decorators

We use decorators mainly, because we already used them since mobx v5 and we find them more descriptive than using annotations. Before 6.0 we were adding decorators to the base class using a helper function, but that did not work any more with v6.

A workaround would be to switch to annotations completely, but as I said, we would like to stick with decorators since those really show up where the props/fields are defined. The annotations are used solely by the helper function, so usually frontend devs would never see them.

The more general use case is that whenever you want to make a set of fields observable, e.g. fields from an interface, with a naming convention, ..., then you will need to use annotations. But in general, decorators are in my opinion the better suited approach for marking specific props/fields - even if decorators are now probably not the recommended way in mobx any more as they used to be with v5.

[urugator]

Who calls that helper function? Frontend devs in their constructors? Why don't you replace that common interface with (abstract) class, decorate it's members as usual, call makeObservable(this) in it's constructor and let the front end dev extend the class instead of implementing interface?

[ahoisl]

We already extend the class, see the "Our use case" from my last comment. The problem is that the base class is in the backend which does not have mobx as a dependency - and should not have it.

Summed up:

• base class in backend (no mobx)
• extended in frontend by making all base class fields observable -> helper function called in constructor that calls makeObservable with annotations
• add custom observable/computed fields/props using decorators -> call makeObservable in constructor without annotations

[urugator]

I see, I would still try to consider some other options, but you can have a helper function that applies decorators programmatically. Decorator is just a normal function, invoked as decorator(prototype, prop).

Either in v5's decorate style, outside class:

class Foo {
  field1 = 42;
};
observable(Foo.prototype, 'field1'); // should be like `@observable field1`

So eg in your front end you can import the undecorated backend class, decorate it like this and reexport for frontend subclasses (well you don't even have to reexport, because it directly modifies the prototype). You have to make sure that someone calls makeObservable(this) in the subclass, otherwise you would have to:

import { BackendBaseClass } from 'backend';

// Note this directly modifies the prototype, so make sure to do it only here
decorateForFrontEnd(BackendBaseClass);

export class ObservableBackendBaseClass extends BackendBaseClass {
   constructor() {
      super(this);
      makeObservable(this);
   }
}

Actually I think you can decorate ObservableBackendBaseClass instead of BackendBaseClass, but it's really late here so I am not entirely sure atm.

Or in v6's style, inside constructor, but you have to make sure to call it just once per class:

const isDecoratedSymbol = new Symbol();
class Foo {
  field1 = 42;
  constructor() {
    const proto = Object.getPrototypeOf(this);
    if (!proto.hasOwnProperty(isDecoratedSymbol)) {
      observable(proto, 'field1');
      Object.defineProperty(proto, isDecoratedSymbol);
    }
  }
};

[ahoisl]

We used to have such a helper function that decorates all fields of a class, but we needed to switch to annotations when migrating to v6.1 - I'm not exactly sure any more what wasn't working any more. I believe it was something with useDefineForClassFields and the field initialization time...

Anyway, you're basically telling me that we are doing something that is not supported and hence not tested, so it's probably better to switch to some supported way...
-> I will try changing our existing helper function from using annotations to decorators. If that does not work, I will probably recommend switching to annotations altogether, even if I don't like it.

I will updated this issue with the outcome in case somebody else has a similar problem. Can you leave the issue open for now?
And thank you for your support!

[urugator]

decorates all fields of a class

If you mean all in a sense that you tried to resolve these fields dynamically, then it must be done in constructor as fields do not exist on the class (prototype). That's why makeObservable must be called in constructor (unless you rely on some TS specific reflection feature). Should be unrelated to useDefineForClassFields though. If you setup a codesandbox I can tinker with, I may try to help.

not supported and hence not tested

Unfortunately that's the case. Tbh I don't remember if there is a fundamental problem or if it's just to avoid complicating things any further without solid justification. The number of possible usage scenarios tends to grow quite fast here and it's likely we will miss that particular one that breaks things and I can guarantee someone will eventually hit this "unthinkable" case :)
Anyone is free to send a PR and convince others, that all is good and well. I personally don't want to take this responsibility atm.

does not have mobx as a dependency - and should not have it

I dunno what the motivation here is, but design wise, I think it would be fine having mobx as a depedency on "backend". Quotes are intentional, because the class is designed to be isomorphic. It is part of the frontend, in the same way as it is part of the backend - you can't change it's implementation without affecting both. It's implementation either pretends that it doesn't need to be observable or it assumes that the observability can be provided later by frontend, which is fine, but the assumption is still part of it's design - the frontend implementation details still leak into the backend.
Don't want to go into lengthy discussions here. Just saying it's an option I personally would take into consideration, especially if it could decrease complexity.

leave the issue open for now

sure

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ahoisl]

Sorry for the late response, I did not get to this issue any earlier.

I have now changed our helper function to use decorators internally and it works well - just like the former helper function using annotations. So this seems to suite and fix my use case. Thank you!

[patrickomeara]

I have recently upgraded to the most recent mobx and am now getting this error. The underlying routing package we use https://github.com/nareshbhatia/mobx-state-router uses mobx without decorators, however we use it with decorators.

Does anyone have a solution to the situation?

I assume this is going to be an issue for a lot of mobx packages that use the alternate to what the developer is using.

[ahoisl]

Just adding that you can mix decorators and annotations, just not for the same observable object, i.e. for one makeObservable call. Sometimes, annotations are easier to use IMHO, e.g. when you want to make specific props in a base class observable without the need to re-declare those props.

[urugator]

@patrickomeara As pointed out by @ahoisl, unless you're extending 3rd party class/object, it shouldn't matter. Please share more details or ideally provide reproduction.

[patrickomeara]

I am extending a 3rd party class that uses annotations, we use decorators in our codebase. However I'm not declaring any observables in the extended class, just overriding a couple methods to give better typing in our application layer.

import { Route, RouterState, RouterStore as NareshRouterStore } from 'mobx-state-router';

export default class RouterStore extends NareshRouterStore {
    constructor(routes: Route[], notFoundState: RouterState, options?: { rootStore: BaseRootStore }) {
        super(routes, notFoundState, options);
    }

    public goTo(
        routeName: string,
        params: { [key: string]: any } = {},
        queryParams: { [key: string]: any } = {}
    ): Promise<RouterState> {
        return super.goTo(routeName, { params, queryParams });
    }
}

[urugator]

just overriding a couple methods to give better typing in our application layer.

In such case you should only need override, which is optional atm, therefore you don't have to do anything. Just double check isAction(routerStore.method) yields true.

Alternatively use composition instead of inheritance.

[patrickomeara]

Thanks @urugator that has worked.

[derekcannon]

@urugator My team and I are using MobX React Form (https://github.com/foxhound87/mobx-react-form), which provides a Form to extend. In our quest to get on the latest and greatest version of MobX, we've come across this "second arg must be nullish when using decorators" issue as well.

MobX React Form uses annotations, but we decorators. Does this mean that library creators will have to export two copies of extendable classes? One with decorator support and one with annotation support?

Or are there other strategies that don't involve having to use whatever methodology library authors create?

(I'm not sure if the upgrade to use the new Stage 3 decorators will change anything in this regard #3638)

[urugator]

Does this mean that library creators will have to export two copies of extendable classes?

It probably means library APIs shouldn't rely on inheritance.
When they do, they don't have to make the class instance itself observable:

class Internal {
  @observable foo = "";
  @action bar() {}

  constructor() {
    makeObservable(this)
  }
}

class Extendable {
    constructor() {
       this.internal = new Internal(); // or just observable({ foo: "", bar() {} }) 
    }
    get foo() {
      return this.internal.foo;
    } 
    bar(...args) {
      return this.internal.bar(...args);
    }
}