Action is not transactional for computeds, but docs says it should be

[jheeffer]

Intended outcome:

Computeds will not be recomputed during an action, as per the docs on action

No observers will be updated until the outer-most action has finished, guaranteeing that intermediate or incomplete values produced during an action are not visible to the rest of the application until the action has completed.

and transaction.

Used to batch a bunch of updates without notifying any observers until the end of the transaction.

and old action docs

actions will batch mutations and only notify computed values and reactions after the (outer most) action has finished.

Now, this is assuming that the term observer is used as in the rest of the code base: computeds & reactions. So not the mobx-react terminology, where it is just a reaction.

Actual outcome:

Computeds get recomputed during an action.

How to reproduce the issue:

https://jsfiddle.net/jasperh/w6bkthu9/8/

const { observable, autorun, action } = mobx;

const obs = observable({
  first: 'John',
  last: 'Doe',
  get full() {
  	return this.first + ' ' + this.last;
  },
  update: action(function(e) {
    this.first = 'Pete';
    console.log('during action', this.full);
    this.last = 'Moe';
    console.log('during action', this.full);
  }),
})

autorun(() => console.log('reaction', obs.full));

obs.update();

Gives console:

reaction John Doe
during action Pete Doe
during action Pete Moe
reaction Pete Moe

Versions

MobX 5 & 6

extra info

Found this while researching a problem I described in this discussion.

I guess docs should be updated that only reaction and not computed is postponed.

Though I'd still like an action that is fully transactional, including for computeds.

[mweststrate]

computed is postponed until the end of the transaction, *unless* it is read by something before the end of the transaction, to make sure no inconsistent world is shown ever, and computed values can be read safely as input for further decision logic. If you remove the `this.full` in your console.logs, and instead put a console.log in full itself, you will see it won't be called eagerly.

…

On Tue, Jun 29, 2021 at 12:32 AM Jasper ***@***.***> wrote: *Intended outcome:* During the execution of an action, computeds will not be recomputed, as per the docs on action <https://mobx.js.org/actions.html> No observers will be updated until the outer-most action has finished, guaranteeing that intermediate or incomplete values produced during an action are not visible to the rest of the application until the action has completed. and transaction <https://mobx.js.org/api.html#transaction>. Used to batch a bunch of updates without notifying any observers until the end of the transaction. and old action docs <https://www.mobxjs.com/refguide/action.html> actions will batch mutations and only notify computed values and reactions after the (outer most) action has finished. Now, this is assuming that the term observer is used as in the rest of the code base: computeds & reactions. So not the mobx-react terminology, where it is just a reaction. *Actual outcome:* Computeds get recomputed during an action. *How to reproduce the issue:* https://jsfiddle.net/jasperh/w6bkthu9/8/ const { observable, autorun, action } = mobx; const obs = observable({ first: 'John', last: 'Doe', get full() { return this.first + ' ' + this.last; }, update: action(function(e) { this.first = 'Pete'; console.log('during action', this.full); this.last = 'Moe'; console.log('during action', this.full); }), }) autorun(() => console.log('reaction', obs.full)); obs.update(); Gives console: reaction John Doe during action Pete Doe during action Pete Moe reaction Pete Moe *Versions* MobX 5 & 6 ** extra info ** Found this while researching a problem I described in this discussion <#3007>. I guess docs should be updated that only reaction and not computed is postponed. Though I'd still like an action that is fully transactional, including wrt computeds (and then maybe even observable properties?). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3017>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBG7BNYPVWEIAFPFXKLTVEBCLANCNFSM47O6JQ2Q> .

[jheeffer]

Sure, but... computeds are never called eagerly, they're always lazy, regardless of transaction context?
It's just reactions that are postponed, which in their turn could call the lazy computeds, right?
I.e. (trans)action doesn't directly influence computed behavior.

In any case, if this is expected behaviour, docs should maybe say:

Action:

No reactions will be run until the outer-most action has finished, guaranteeing that intermediate or incomplete values produced during an action are not visible to the rest of the application until the action has completed. However, computeds will update when they have become stale and are read during an action.

Transaction:

Used to batch a bunch of updates without running any reactions until the end of the transaction.

Since at least computeds seemingly áre notified, they just remain lazy, like they always are; They will recompute when stale & read (in either an action or a reaction).

I can make a PR for that?

Would you be interested to include a computed-included transaction functionality in mobx ( I could try, though it seems a deep dive :D)? If not, can you give me a pointer how I could implement it myself?

[mweststrate]

I can make a PR for that?

Sure!

Would you be interested to make a computed-included transaction functionality?

Nope, such a model could become easily too finicky to reason about and support. E.g. some code reading a computed would suddenly become buggy because a (totally unrelated and unaware) transaction higher in the stack changes the behavior of computeds. Note that you can set this option to forbid uncached reads: https://mobx.js.org/computeds.html#requiresreaction. I'd use it only very sparingly.

[jheeffer]

Oh yeah, I didn't mean to change current transaction's behavior, I meant a different transaction that pauses computed updates for e.g. a new actionStatic() API method. It would add to API surface though, so I understand that with a limited use case you don't want to add it.

Do you reckon it would take a big refactor or should it be a relatively small change, if I'd try to do it myself?

Note that you can set this option to forbid uncached reads: https://mobx.js.org/computeds.html#requiresreaction. I'd use it only very sparingly.

Thanks, though I'm not sure how that would help in this situation, I'm always in reactive context when reading.

PR for docs: #3018

[mweststrate]

What I mean is that the behavior of computed would become unpredictable, since it will be influenced now by something that might be happening completely somewhere else. Give the following code:

class Temp {
   degrees = 0

   @computed get degreesFahrenheit() {
      return this.degrees + 123
    }
    
    @action setDegrees(temp) {
        this.degrees = temp
        if (this.degreesFahrenheit > 456) {
           alert("too hot for humans")
        }
    }
}

In the above example it will now become completely unpredictable what degreesFahrenheit is seen, and hence whether the alert triggers or not, because you can't tell from which kind of transaction setDegrees is called, since that might be determined by arbitrarily logic higher up in the stack. So reasoning about the correctness of the applications becomes very complicated with the proposed kind of transaction.

Note that if computation postponement is really important in your specific case, you can convert the computed to a reaction + observable: reaction(() => this.degrees, degrees => { this.degreesFahrenheit = degrees + 123 })

[jheeffer]

Right, I understand, that's insightful, thanks for elaborating : ). Logic of setDegrees would behave differently depending on what kind of transaction calls it. That's definitely complicated and unwanted.

Here's a fibonacci example where the computed-updates transaction can cause confusion if you're not very familiar with the reactive paradigm, as normally the action's code would work. The solution is of course just dereferencing this.n before assigning anything. In this case it's a simple fix, in other cases it may be less obvious.

https://jsfiddle.net/jasperh/w6bkthu9/25/

const { observable, autorun, action, toJS } = mobx;

const fib = observable({
  n_min1: 1,
  n_min2: 1,
  i: 2,
  get n() {
    return this.n_min1 + this.n_min2;
  },
  update: action(function() {
    this.i++;
    this.n_min2 = this.n_min1;
    this.n_min1 = this.n;
  }),
})

autorun(() => console.log(`fib no ${fib.i}: ${fib.n}`, toJS(fib)));

fib.update();
fib.update();
fib.update();

Note that if computation postponement is really important in your specific case, you can convert the computed to a reaction + observable: reaction(() => this.degrees, degrees => { this.degreesFahrenheit = degrees + 123 })

Yup, but now other reactions can get inconsistent state (celcius <-> fahrenheit) if they run before this reaction. And we can't control reaction order. Same problem as I mentioned in the discussion before the weekend.

I guess I'm just going with the current behaviour and tell users of our library to dereference any state before updating state in the actions they write.

[jheeffer]

Logic of setDegrees would behave differently depending on what kind of transaction calls it.

I guess you can let each action dictate its own behavior. Anything set in setDegrees always makes computeds stale right away. Anything set directly in a "static" action (not in actions further down the stack) doesn't make computeds stale (yet). But yeah, regardless, it adds additional mental strain:

• To think about what type of action you're calling/writing.
• What are boundaries for a static action's computed-staleness-postponing when it's called by normal action?
• How does a computed deal with updates from both types of actions.

[mweststrate]

The solution is of course just dereferencing this.n before assigning anything.

MobX behaves here no differently than how JavaScript itself would behave, so I don't see really what there is to warn about :)

[jheeffer]

Right, if you keep this.n as a getter; I see your point :).

I was thinking more of a non-mobx implementation that would go for this.n being normal state and set in the action after the other assignments (jsFiddle). Or a good old fibonacci-for-loop. Guess I'm not used to thinking about getters in vanilla JS : ). Maybe a good thing :P

In our case we need to warn since it's less obvious than this example. In short: accessing a model also observes a model.state computed, which observes a fromPromise computed, which observes props and sends an async request . If you set one prop, that computed chain becomes stale. When you then access the model again to set another prop, the state read triggers the async request prematurely (it should wait for end of action).

Sending async in a reaction is not an option either because we can't control order of reaction execution.

I've tried to think of other refactors that would remove the state reading in model access. That would solve it too, for now at least, alas no solution found yet : ).

[jheeffer]

I'll close this as the issue itself is solved.