Atom onBecomeUnobserved retriggered when it changes observable map

[ptomask]

Greetings. I found a possible issue with Atom. I expect following code to print out Value getting unobserved only once, but it is printed out twice. The onBecomeUnobserved handler deletes from observable map, which is tracked in @computed value getter and I think in consequence retriggers the handler, which should not happen. Reproduced here - see the console.

In practice the code should delete an item (someKey) from cache (myMap) when that item is no longer needed by anything.

class ClassA {
  myMap = observable.map({
    someKey: 'someKeyValue',
  });

  constructor() {
    this._atom = new Atom(
      'Testing atom',
      () => {
        console.log('Value getting observed.')
      },
      () => {
        console.log('Value getting unobserved.')
        this.myMap.delete('someKey') // Retriggers onUnobserved handler?
      }
    )
  }

  @computed get value() {
    this._atom.reportObserved();
    return this.myMap.get('someKey');
  }
}


const a = new ClassA();

const unobserve = autorun(() => {
  console.log(a.value);
})

setTimeout(unobserve, 1000) // Consequently displays "Value getting unobserved" twice.

[urugator]

I've simplified the problem:

The handler is retriggered once per each observable modified inside the onUnobserved handler
If a single observable is modified more than once, handler invocation ends up in infinite recursion.
(type of the observable does not matter, observable does not have to be tracked, computed is not needed)

const Mobx = require("mobx");

class ClassA {  
  constructor() {
    this.prop1 = Mobx.observable(1); 
    this.prop2 = Mobx.observable(2);   
    this._atom = new Mobx.Atom(
      'Testing atom',
      () => {
        console.log('Value getting observed.')
      },
      () => {
        console.log('Value getting unobserved.')        
        this.prop1.set(11); // retriggers once
        this.prop2.set(22); // retriggers once again
        //this.prop1.set(111); // <-- INFINITE RECURSION
      }
    )
  }  
  get value() { // computed not needed
     // observables don't have to be tracked
    this._atom.reportObserved();    
  }
}

const a = new ClassA();
const unobserve = Mobx.autorun(() => {
  console.log(a.value);
})
setTimeout(unobserve, 1000) 

Interestingly, to reproduce the same problem we don't have to modify any observable, we can just invoke an empty action:

const Mobx = require("mobx");

class ClassA {  
  constructor() {    
    this._atom = new Mobx.Atom(
      'Testing atom',
      () => {
        console.log('Value getting observed.')
      },
      () => {
        console.log('Value getting unobserved.')              
        Mobx.runInAction(() => {}); // <-- INFINITE RECURSION
      }
    )
  }  
  get value() {
    this._atom.reportObserved();    
  }
}

const a = new ClassA();
const unobserve = Mobx.autorun(() => {
  console.log(a.value);
})
setTimeout(unobserve, 1000) 

[mweststrate]

Fixed by 3.3.1

[urugator]

@mweststrate You sure? I just tried both of my code samples with 3.3.1 and the behavior seems the same...

[mweststrate]

Hmm a test for it was added. Could you otherwise create a PR with additional tests?

…

On do 5 okt. 2017 21:16 urugator ***@***.***> wrote: @mweststrate <https://github.com/mweststrate> You sure? I just tried both of my code samples with 3.3.1 and the behavior seems the same... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhF8u1OakJo4gr_02OEpPBhOMNSKcks5spSsOgaJpZM4OutyL> .

[urugator]

Can you point me to the relevant commit?

[mweststrate]

ad81239 But now i see i commented in the wrong thread 😐. So please reopen 😊

…

On do 5 okt. 2017 21:28 urugator ***@***.***> wrote: Can you point to me to the relevant commit? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1122 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhBhaZMrfUgp6sDyRwqmf7bbWZ_7sks5spS35gaJpZM4OutyL> .

[mweststrate]

Added the test case to the MobX 4 branch, and the new Atom implementation seems to have fixed it.