mobxjs / mobx-utils

Utility functions and common patterns for MobX
MIT License
1.18k stars 124 forks source link

now() returns not a relevant time when no active subscriptions #330

Open vovkasm opened 3 months ago

vovkasm commented 3 months ago

There is a fix in #40, but it seems broken now, code:

export function now(interval: number | "frame" = 1000) {
    if (!_isComputingDerivation()) {
        // See #40
        return Date.now()
    }
 ...

In case no subscriptions, _isComputingDerivation always return false. I am not sure why, even if wrap now() in action.

Reproduction:

import {makeAutoObservable, reaction} from "mobx";
import {now} from "mobx-utils";

class Counter {
  stopTime: number = 0;

  get remaining() {
    const delta = this.stopTime - now(1000);
    return delta > 0 ? delta : 0;
  }

  get done() {
    return this.remaining <= 0;
  }

  constructor() {
    makeAutoObservable(this);
  }

  start(duration: number) {
    this.stopTime = Date.now() + duration;
  }

  imperativeCheck() {
    console.log(`imperativeCheck: remaining=${this.remaining} done=${this.done}`);
  }
}

const counter = new Counter();
const disposer = reaction(
  () => counter.remaining,
  (remaining) => {
    console.log(`reactiveCheck: remaining=${remaining}`);
  },
);

console.log("start timer for 10s");
counter.start(10 * 1000);
let imperativeCheckCounts = 0;
scheduleImperativeCheck();

function scheduleImperativeCheck() {
  imperativeCheckCounts++;
  setTimeout(() => {
    counter.imperativeCheck();
    if (imperativeCheckCounts === 2) {
      console.log("Now we remove reaction");
      disposer();
    }
    if (!counter.done) scheduleImperativeCheck();
  }, 2000);
}

Result:

start timer for 10s
reactiveCheck: remaining=10000
reactiveCheck: remaining=8998
imperativeCheck: remaining=8998 done=false
reactiveCheck: remaining=7994
reactiveCheck: remaining=6993
imperativeCheck: remaining=6993 done=false
Now we remove reaction
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
imperativeCheck: remaining=6993 done=false
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
imperativeCheck: remaining=6993 done=false
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
imperativeCheck: remaining=6993 done=false
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started
imperativeCheck: remaining=6993 done=false
Called `get` of a subscribingObservable outside a reaction. Current value will be returned but no new subscription has started

Expected behaviour: After removing reaction, imperativeCheck should log decreasing times and stop...

Note: this is a somewhat artifical example. In the actual application we have some logic based on the current time, which is used in many places, some of which are in the reactive context, whereas others are not. In theory, we might write this logic twice: one with now() from mobx-utils, and the other with Date.now(). However, it is a huge duplication of code and makes the code unreadable.

vovkasm commented 3 months ago

I'm just tried this simplified implementation (embedded within same test script):

import {createAtom, makeAutoObservable, reaction} from "mobx";
// import {now} from "mobx-utils";

const {now} = mkClock();

class Counter {
  stopTime: number = 0;

  get remaining() {
    const delta = this.stopTime - now();
    return delta > 0 ? delta : 0;
  }

  get done() {
    return this.remaining <= 0;
  }

  constructor() {
    makeAutoObservable(this);
  }

  start(duration: number) {
    this.stopTime = Date.now() + duration;
  }

  imperativeCheck() {
    console.log(`imperativeCheck: remaining=${this.remaining} done=${this.done}`);
  }
}

const counter = new Counter();
const disposer = reaction(
  () => counter.remaining,
  (remaining) => {
    console.log(`reactiveCheck: remaining=${remaining}`);
  },
);

console.log("start timer for 10s");
counter.start(10 * 1000);
let imperativeCheckCounts = 0;
scheduleImperativeCheck();

function scheduleImperativeCheck() {
  imperativeCheckCounts++;
  setTimeout(() => {
    counter.imperativeCheck();
    if (imperativeCheckCounts === 2) {
      console.log("Now we remove reaction");
      disposer();
    }
    if (!counter.done) scheduleImperativeCheck();
  }, 2000);
}

function mkClock() {
  let current = Date.now();
  let timer: ReturnType<typeof setInterval> | undefined;
  const atom = createAtom(
    "Clock",
    () => {
      tick();
      timer = setInterval(tick, 1000);
    },
    () => {
      clearInterval(timer);
      timer = undefined;
    },
  );
  function tick() {
    current = Date.now();
    atom.reportChanged();
  }
  function getNow() {
    return atom.reportObserved() ? current : Date.now();
  }
  return {now: getNow};
}

It works very well and report to console as expected, moreover no subscriptions existed after countdown and script successfully exit:

start timer for 10s
reactiveCheck: remaining=10000
reactiveCheck: remaining=8999
imperativeCheck: remaining=8999 done=false
reactiveCheck: remaining=7996
reactiveCheck: remaining=6995
imperativeCheck: remaining=6995 done=false
Now we remove reaction
imperativeCheck: remaining=3989 done=false
imperativeCheck: remaining=1984 done=false
imperativeCheck: remaining=0 done=true
AfanasievN commented 3 months ago

UP