Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vovkasm opened this issue Jun 5, 2024 · 2 comments
Open

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

vovkasm opened this issue Jun 5, 2024 · 2 comments

Comments

@vovkasm
Copy link

vovkasm commented Jun 5, 2024

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
Copy link
Author

vovkasm commented Jun 5, 2024

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
Copy link

UP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants