Mobx throws error message when changing observable values in the constructor in certain situation

[breck7]

Intended outcome:

I expect no side effect error when I change an observable value in the constructor.

Actual outcome:

Error: [mobx] Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify...

How to reproduce the issue:

import { observable, computed } from "mobx"

class Mall {
    @computed get openStore() {
        return new Store("Open")
    }

    @computed get closedStore() {
        return new Store("Closed")
    }
}

class Store {
    constructor(status: string) {
        this.status = status
        if (this.isOpen && this.shouldClose()) this.status = "Closed"
    }

    shouldClose() {
        return true // Imagine there's some actual logic here
    }

    @observable status: string

    @computed get isOpen() {
        return this.status === "Open"
    }
}

// Works
new Store("Closed")
new Store("Open")
new Mall().closedStore

// Fails
new Mall().openStore

Versions

5.13.0.

Additional Context

I was hitting an Error: [mobx] Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify... and ended up in a lengthy debugging session.

I am not sure if this is a bug or is working as intended, but it was really tricky to debug so I thought I'd post this here in case anyone else runs into a similar situation.

I think my main question is should you never create new instances inside of a computed? Or more specifically should you never create new instances with observables inside of a computed? I didn't see a rule for that, but it's likely I just missed something. If that's something that you shouldn't do, is there a linting rule?

This bug happens when you:

1. Attempt to change an observable property
2. In the constructor
3. After calling a computed in that constructor that references that observable
4. Only when you are instantiating that object in the computed of another object

All 4 of those conditions have to be met, which is why it was tricky to debug. What is surprising to me is that the behavior of new Store("Open") would be different depending upon whether it was called in the scope of a computed or not.

[mweststrate]

It is possible to create observables in computeds, but the good cases are rare, and your above example doesn't look like one of them. The stores in a mall aren't derived values, but stated. So you should rather model that using as observable rather than computeds. Instead of burning down all the stores and create new ones every day when you open the mall, you'd be better of if you just swap the signs on the door, something along these lines. Things that are 'addressable identities' are usually not derived values but state. E.g. 'the starbucks in the mall'. Closing that store doesn't make it a different store altogether, it just changes the state the store is in.

class Mall {
   @observable store = new Store()

   openStore() { this.store.status = "open" }

    closeStore() { this.store.status = "closed" }
}

class Store {
   @observable  status = "closed" 

   constructor(status) {
        this.status = status
    }

    @computed shouldClose() {
        return true // Imagine there's some actual logic here
    }

    @computed get isOpen() {
        return this.status === "Open" && !this.shouldClose()
    }
}

(shouldClose sounds more like a transition that should happen, rather than a state you can find a store in, in which case you might want to model it is an action instead). For more info check: https://mobx.js.org/intro/concepts.html

[breck7]

Thanks @mweststrate ! I guess my contrived test case obscured the issue.

Here's one that more closely mirrors our actual code. In this case:

1. The user can set certain Settings even if they are invalid (we preserve their invalid settings).
2. Dependent code can always derive a valid Settings object from the user's Settings.

import { observable, computed } from "mobx"

declare type choice = "apple" | "none"

class UserSettings {
    constructor(itemA: choice, itemB: choice) {
        this.itemA = itemA
        this.itemB = itemB
    }

    get validSettingsWORKS() {
        const clone = new UserSettings(this.itemA, this.itemB)
        if (clone.itemsAreEqual) clone.itemB = "none"
        return clone
    }

    @computed get validSettingsBREAKS() {
        const clone = new UserSettings(this.itemA, this.itemB)
        if (clone.itemsAreEqual) clone.itemB = "none"
        return clone
    }

    @computed get itemsAreEqual() {
        return this.itemA === this.itemB
    }

    @observable itemA: choice
    @observable itemB: choice
}

// Works
new UserSettings("apple", "apple").validSettingsWORKS

// Throws
new UserSettings("apple", "apple").validSettingsBREAKS

It seems to me very surprising that the exact same code breaks when in a computed. One way around this would be to have a rule of not creating new observables inside of a computed. The problem is here I'd need 2 near identical classes, one with observables and one without. (Unless there is an easy way to make an instance not observable?)

[mweststrate]

The general idea is that computers don't produce new state, as they are derived information and producing stateful information leads to state duplication that has is completely unmanaged, if the computed recomputes a few times, you have many settings objects lingering around, and they all could be in different states, be changed, etc.. So yeah, I wouldn't try to recycle the same class for that, but rather export precisely the information you want to expose as plain object, so that there is always a clear source of truth in your app, rather than many different ones.

[breck7]

That makes sense, @mweststrate. Thanks.

I think it may be helpful to throw in a computed if someone is created new instances with observables. Or call it out specifically in the pitfalls.

In my case I don't actually want to be creating those observables, just a plain instance with unobservable properties. But the class has something like 20 properties, so duplicating that class into one with observables and one without seems bad. Unless there is some type of magical pattern where I could make all properties observable (or vice versa):

class Settings {
 title: string
 status: string
}
@makeAllPropertiesObservable
class ObservableSettings extends Settings { }

[mweststrate]

return { ...mySettings } ?

…

On Wed, Jul 29, 2020 at 10:27 PM Breck Yunits ***@***.***> wrote: That makes sense, @mweststrate <https://github.com/mweststrate>. Thanks. I think it may be helpful to throw in a computed if someone is created new instances with observables. Or call it out specifically in the pitfalls. In my case I don't actually want to be creating those observables, just a plain instance with unobservable properties. But the class has something like 20 properties, so duplicating that class into one with observables and one without seems bad. Unless there is some type of magical pattern where I could make all properties observable (or vice versa): class Settings { title: string status: string } ***@***.*** class ObservableSettings extends Settings { } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2409 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBBGGQPYZRVXDYQG7O3R6CH4TANCNFSM4PISUZ4A> .

[breck7]

It's a good idea but we have a number of helper methods that we'd lose access to.

What I ended up doing is simply ditching the computed and moving the body of the computed to the constructor. So, in the example above I removed itemsAreEqual and just put this.itemA === this.itemB directly in the constructor, which seems to work since then no computeds are observing itemB when I change it.

Happy enough with the solution but feels to me like there is some improvement that can be made (I'm just not sure what that is :) ). And it may be a rare case.