mobx-react(-lite) useLocalObservable will wrap a class instance in observable if make(Auto)Observable is not used

[AoDev]

Context

I am migrating my boilerplates

• from mobx v4/v5 mobx-react v6
• to mobx v6, mobx-react v7

Intended outcome

I switched to mobxReact.useLocalObservable().

For some complex components, I have classes for local state.
So I have adapted the code to have calls like mobxReact.useLocalObservable(() => new MyState())

While most of the time, make(Auto)Observable will be used, I also had the case where a class "in WIP" was just passing non observable data.

import * as mobx from 'mobx'

export default class AppSettingsVM {
  constructor ({rootStore}) {
    mobx.makeAutoObservable(this) // <- WITHOUT this line, the instance is turned into an observable and the app crashes
    this.settings = rootStore.coreStore.settings
  }
}

Actual outcome:

From the docs, if useLocalObservable returns a class instance, it should be kept as is, which is not what is happening here and leads to a crash.

How to reproduce the issue:

Use useLocalObservable where it returns a class instance that is not using mobx make(Auto)Observable.

Versions
mobx v6, mobx-react v7

[mweststrate]

Would you mind creating a small codesandbox to demonstrate the issue?

[danielkcz]

Just a note, there is effectively no reason to use useLocalObservable for class based stores. Instead you can just do const [state] = React.useState(() = new MyState()). That hook does mostly the same, but passes your instance through observable which is unnecesssary.

[AoDev]

@FredyC I naively replaced useLocalStore by useLocalObservable because I had a deprecation warning about useLocalStore. What you say above make total sense, React.useState is enough and I'll use that.

But the issue does exist. The docs say you can return a class instance and it would be kept as is which seems to not be correct. It seems that useLocalObservable treats a class that has been decorated with makeAutoObservable differently than a class that was not.

Maybe we should clarify that useLocalObservable should be used only with objects?

Here is a sandbox where the issue can be reproduced @mweststrate
Note that you might need to reload the sandbox browser after edit.

https://codesandbox.io/s/suspicious-lewin-okerp?file=/src/MyStore.js

You can see the difference when I console.log the local store in the component.

[danielkcz]

I agree with you this is a long time confusion. Class-based stores should not have been used with useLocalStore, but it was more tolerant about it. Maybe mobx observable V6 should ignore decorated class instances, is that possible @mweststrate?

[mweststrate]

Yeah it is what it should do indeed, Im surprised with this behavior, might be a bug but didn't check the sandbox yet Op zo 1 nov. 2020 17:16 schreef Daniel K. <notifications@github.com>:

…

I agree with you this is a long time confusion. Class-based stores should not have been used with useLocalStore, but it was more tolerant about it. Maybe mobx observable V6 should ignore decorated class instances, is that possible @mweststrate <https://github.com/mweststrate>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2579 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDG5GY3BZVSCBQXWA3SNWJYTANCNFSM4TGRULFQ> .

[urugator]

mobx/packages/mobx/src/api/observable.ts

Lines 96 to 108 in 44b6413

	const res = isPlainObject(v)
	? observable.object(v, arg2, arg3)
	: Array.isArray(v)
	? observable.array(v, arg2)
	: isES6Map(v)
	? observable.map(v, arg2)
	: isES6Set(v)
	? observable.set(v, arg2)
	: v
	// this value could be converted to a new observable data structure, return it
	if (res !== v) return res
	return observable.box(v)

It always defaults to boxed observable (even for class instance).

[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.