Mobx: Computed only triggers once

[ktosiek]

Intended outcome:

https://codesandbox.io/s/dark-silence-5r6kh?file=/src/App.tsx:443-526

Having a computed property update each time dependency changes - the component on screen should show a new number every second.

Actual outcome:

The property getter only runs once.

How to reproduce the issue:

I couldn't reproduce the issue with https://codesandbox.io/s/minimal-mobx-react-project-ppgml, but it fails reliably in the linked sandbox. Dropping the @computed fixes the component, but does not fix the autorun at the bottom of the code - this one still only runs once.

Versions

mobx: 4.15.4
mobx-react-lite: 2.0.6
react: 16.12.0

[FredyC]

That goes beyond my expertise, I would have been expecting this to work as well.

@urugator Can you have a look, please?

[urugator]

The problem seems to be line 40 (synchronous mutation from within a computed) in a combination with up-ahead subscription for non-existent key.
The problem disappears either if the map is pre-initialized new ObservableMap([[42, undefined]]) or line 40 this.values.set(k, k); is removed...

I would start looking here:
https://github.com/mobxjs/mobx/blob/master/src/v4/types/observablemap.ts#L128

onBecome(Un)Observed can be called a bit unexpectably as noted here https://github.com/mobxjs/mobx/issues/2309#issuecomment-598707584, but perhaps it's unrelated.

[ktosiek]

Is mutation from within a computed something that is expected to work well? Or should it log a warning/throw an exception?

[mweststrate]

This is a very subtle issue, but indeed the result of doing side effects in computed, which is an anti pattern. It goes like this:

1. anything done in an action will not be tracked by the computed, to prevent introducing unending loops, and clearly separating what-to-react-to versus side-effects
2. So the only thing get(k) subscribes to is the present / absence of key k (since that is the only observable accessed)
3. the fetchValue immediately sets the k key, making k present. However, because the computed didn't finish computed yet, that is not a dependency _yet_.
4. So the problem is now: get never subscribed to the value of k because it was irrelevant during the first computation of get (as it didn't exist yet). However, it will never run again because the presence of k is not going to change anymore _after_ the computation finishes (since it is already set during that first computation). So commenting out that first this.values.set(k, k) fixes the issue here, because that is a change which cannot be observed by the computation.

I guess this nicely highlights the risk of mixing derivations and updates :) But maybe we should warn more aggressively here.

_EDIT: so, basically what @urugator said, totally missed his comment :/_

[urugator]

@mweststrate Why observer and autorun behaves differently when @computed is removed?

[ktosiek]

The behaviour is different because without @computed the problem only happens for the one that calls get42 first. The second one doesn't trigger the "if undefined set default" path.

[urugator]

You sure? when I comment out the autorun the component is still updating... there is immediate re-render after first fetchValue...

[melnikov-s]

Here's a simpler reproduction of the same underlying issue:

https://codesandbox.io/s/minimal-mobx-react-project-zikbo?file=/index.js:0-331

Seems to be working as expected for reasons already mentioned, only potential improvement I can see here would be to throw a "cycle detected" error as technically the set operation on line 8 should (in theory, not according to current implementation) re-trigger the computed.

[urugator]

I gave it some thought and imo get should always subscribe for value, not just for the presence/absence of key.
If the value doesn't exist yet, we should pre-create it and subscribe for it:

get(key) {    
  if (this.has(key)) return this.dehanceValue(this._data.get(key)!.get())  
  // If the key doesn't exist yet  
  // prepare "virtual" value
  if (!this.virtualDataMap.has(key)) {
    const observableValue = new ObservableValue(
       undefined,
       this.enhancer,
       `${this.name}.${stringifyKey(key)}`,
       false
    )   
    this.virtualDataMap.set(key, observableValue);
  } 
  // Subscribe for virtual value
  return this.virtualDataMap.get(key).get();
}

set(key, value) {
  // If this is a virtual value move it to real data map
  if (this.virtualDataMap.has(key)) {    
    const observableValue = this.virtualDataMap.get(key);
    this._data.set(key, observableValue) 
    this.virtualDataMap.delete(key);
  } 
  // Rest is basically the same...
  // Inside _addValue we will just update the observable inside `dataMap` instead of creating a new one
  // Note, when value===undefined, the reportChanged isn't called, which is OK, because it won't affect the result of `get` (it remains undefined, even thought it no longer represents missing value)
}

This should solve the problem, because now the computed depends on the value and so if the value truly changes later (depending on what it was initialized to inside computation) it will notify the computed.

[MoonBall]

@mweststrate I have an idea. We can throw an error if user call an action while computing a compute value.
we can add a isComputing state to globalState for implementing the idea.

[mweststrate]

Looked into this one, and I don't think there is an easy / clear solution that doesn't overhaul maps, or significantly change it's mem/cpu consumption.

But looking closer at your example, the initial return undefined seems to be incorrect as well, since the first run should (imho) already return k. Changing get accordingly fixes both problems at once and avoids the otherwise double autorun :):

    get(k) {
        if (this.values.has(k)) return this.values.get(k)
        this.fetchValue(k);
        return this.values.get(k); // instead of `undefined` and takes care of subscription!
    }