Bug in Example domain store code? - infinite reaction loop

[keriwarr]

tl;dr: Two-way data binding in example domain store code leads to infinite reaction loop - this.autoSave doesn't work as intended when called inside an action which is difficult to categorically avoid. I'm not sure what the fix should be.

---

I was playing around with this the Example domain store code, and I ran into an issue after hooking it up to a real transport layer - when I update a todo, it ends up in an infinite update loop. The reason is this:

1) transportLayer triggers updateTodoFromServer 2) which calls updateFromJson (which is an action!) 3) which sets autoSave to false 4) and then updates some todo values 5) and then sets autoSave to true 6) which only then triggers save handler. since autoSave has already been restored before the action finished

You can bandage this by including updateFromJson in the makeAutoObservable exclude list (and wrap the property updates in an a runInAction to prevent spamming reactions). The problem with this fix is that if you ever forget about this issue and call updateFromJson inside of an action (really easy to do in mobx IMO) you now have an infinite network call loop! I would consider this minimally solved if I could, inside updateFromJson say "if we're inside an action, throw an error", but that doesn't seem to be a capability.

Here's a reproduction of the issue with just enough extra code added to the example to demonstrate the above: https://codesandbox.io/s/adoring-jasper-v5s7li

check out the console to observe that saveTodo is called in response to updateTodoFromServer. Note that there is no infinite loop in this case because saveTodo doesn't do anything other than log to the console.

I'm reporting this as an issue because (setting aside the fact that afaict updateFromJson was mistakenly omitted from the exclude list) this whole two-way binding setup feels a bit too thorny as is, and I haven't yet figured out what a good alternative would be.

My apologies in advance if I got it wrong. 😆

As always, thanks for your hard work.

[urugator]

I think you're correct the example is wrong. Maybe I am missing something , but the whole idea that you can supress reaction like this seems weird.

Perhaps it was meant to be like following? Dunno.

this.saveHandler = reaction(
      () => this.asJson, // Observe everything that is used in the JSON.
      (json) => {
        // If autoSave is true, send JSON to the server.
        if (this.autoSave) {
          this.store.transportLayer.saveTodo(json);
        } else {
          this.autoSave = true
        }
      }
    );

updateFromJson(json) {
    this.autoSave = false; // Prevent sending of our changes back to the server.
    this.completed = json.completed;
    this.task = json.task;
    this.author = this.store.authorStore.resolveAuthor(json.authorId);
 }

A good alternative is not using reactions :)