Converting functions to actions (autoActions?) in deep enhancer leads to bugs

[Obi-Dann]

We're upgrading from an ancient (4) version of mobx to Mobx 6 and encountering hard to detect issues. It's mainly related to the decision to wrap every function to action that's passed directly to observable or inside an object that was converted to deep observable. Here's the changelog record https://github.com/mobxjs/mobx/blob/main/packages/mobx/CHANGELOG.md#620

First of all, the value of the object changes, if there were additional properties assigned on the function object, they would not be accessible, eg:

class Store {
  @observable accessor fn: () => void;

  constructor() {
   const f = () => {console.log('a')};
   Object.assign(f, {meta: 'foo'});
   this.fn = f;

   console.log(f.meta); // 'foo'
   console.log(this.fn.meta); // undefined

   this.fn === f; // false - this is weird
  }
}

Some may argue that it's an anti pattern but it does lead to unexpected behavior which is more confusing because the property wasn't marked explicitly as an action.

I though it might lead to issues with reactions because actions are (or used to be) untracked, but it seems to work OK.

A more real world test case when dealing with an array of functions Removing the original fn from the array does nothing, which makes it impossible to use observable arrays for pubsub style patterns.

const fn = () => false;

const fns = observable.array();

fns.push(fn);
fns.remove(fn);

console.log("length", fns.length); // length 1

My main issue with this behavior is that it's obscure, it changes the logic in code and leads to hard to trace bugs. mobx should not wrap functions into actions by default.

We can use observable.ref or shallow or something like that, but this issues are hard to find and reason about. The default behavior should not introduce bugs like that.

Intended outcome:

const fn = () => false;

const fns = observable.array();

fns.push(fn);
fns.remove(fn);

console.log("length", fns.length); // length 0

Actual outcome:

const fn = () => false;

const fns = observable.array();

fns.push(fn);
fns.remove(fn);

console.log("length", fns.length); // length 1

How to reproduce the issue: Created a stackblitz sandbox with unit tests showcasing the issue

https://stackblitz.com/edit/vitejs-vite-y23sku?file=src%2Fmobx.test.ts

Versions Mobx 6+ ?

[urugator]

My main issue with this behavior is that it's obscure

It's obscure in exactly the same way as with any other non-primitive value in mobx:

const ob = {};

const obs = observable.array();

obs.push(ob);
obs.remove(ob);

console.log("length", obs.length); // length 1

So one could argue it's more consistent now.

makes it impossible to use observable arrays for pubsub style if there were additional properties assigned on the function object

You can still do that, but you have to convert the value to action before attaching extra props or placing to observable:

const fn = action(() => false); // or autoAction
fn.foo = 'foo';

const fns = observable.array();

fns.push(fn);
const sameFn = fns[0];
fns.remove(fn);

console.log("length", fns.length); // length 0
console.log("fn.foo", sameFn.foo); // foo

Also see: https://mobx.js.org/migrating-from-4-or-5.html#functions-are-auto-converted

[Obi-Dann]

Wrapping functions into actions seems worse than wrapping objects, because additional fields on functions won't be available... which won't be reflected in typescript

const fn = observable(Object.assign(() => {}, { meta: 'bleh' }));
console.log(fn.meta.length); // Typescript is happy, but it'd be an error at runtime

There's no problem like that with objects because their properties are available after converting into observable

Essentially, now we need to work around this mobx "feature" and likely add custom ESLint rules to ensure mobx is used properly. Not sure how it helps to reduce friction with autoactions

[urugator]

Yes, that's an oversight. I assume it's fixable for observable(fn), but I don't know about deep conversions done by enhancer as we seem to have the same problem with Array/Set/Map:

const obj = observable(
    Object.assign(
      {
        arr: Object.assign([], { meta: 'bleh' }),
        set: Object.assign(new Set(), { meta: 'bleh' }),
        map: Object.assign(new Map(), { meta: 'bleh' }),
      },
      { meta: 'bleh' }
    )
  );
  console.log(obj.meta);
  console.log(obj.arr.meta); // Typescript is happy, but it'd be an error at runtime
  console.log(obj.set.meta); // Typescript is happy, but it'd be an error at runtime
  console.log(obj.map.meta); // Typescript is happy, but it'd be an error at runtime

[mweststrate]

This is working as intended and carefully designed through address several issues, to create more consistent defaults as much as possible, but as with all designs there were tradeoffs :).

However, it is possible to opt-out by specifying annotations in practically all parts of the API by disabling deep observability. I've updated the blitz to reflect what it would look like to make the tests pass: https://stackblitz.com/edit/vitejs-vite-umg6bj?file=src%2Fmobx.test.ts

[Obi-Dann]

Thanks for the answers, gonna close it as it's intended behavior