Use @computed({keepAlive: true}) without autorun

[hector]

I would like this to work:

class Foo {
  @observable stuff = 'acme';

  @computed get coolStuff() {
    return expensiveCalculationWithStuff();
  }

}

const foo = Foo();
console.log(coolStuff); // expensiveCalculationWithStuff() is called
console.log(coolStuff); // expensiveCalculationWithStuff() is NOT called: use cached value

I have learned from other issues (#1093) that the cached value of a computed is only kept while something is observing it.

The suggested solution is to use @computed({keepAlive: true}). This works but it calculates the computed value on object creation, not when trying to read the computed value for the first time. This is because keepAlive: true creates an autorun:

mobx/src/core/computedvalue.ts

Line 126 in 8fb2861

autorun(() => this.get())

This seems wrong since it breaks the description of a computed:

1. First time it's being accessed it will compute and remember result

Trying solutions I have found that using observe instead of autorun works fine: it caches the value but it does not calculate it before it is needed. Should this change be done?

The only concern I have is if observe will get disposed when foo is garbage collected.

[urugator]

Eager behavior is not very practical indeed. (btw it was already mentioned here)
I quess it wasn't implemented, because it would require to change the implementation of computed itself and added complexity wouldn't worth the limited usage of keepAlive? dunno...

I have found that using observe instead of autorun works fine: it caches the value but it does not calculate it before it is needed.

You sure? Check the console https://jsfiddle.net/nLev2ozo/1/

As a workaround you can create autorun lazily by yourself

class Foo {
  @observable stuff = 'acme';  
    
  @computed get _coolStuff() {
    /* expensive computation */
  }  
  
  get coolStuff() {
    if (!this._coolStuffDisposer) {
      this._coolStuffDisposer = autorun(() => this._coolStuff);      
    }
    return this._coolStuff;
  }
  
  // Optionally expose disposer
  disposeCoolStuff() {
    this._coolStuffDisposer();
    this._coolStuffDisposer = null; 
  }
}

[hector]

You sure? Check the console https://jsfiddle.net/nLev2ozo/1/

You are right, I was trying things late at night. observe() doesn't seem to keep the computed value cached:
https://jsfiddle.net/fd4faz6c/

So I see 2 problems here:

First

The documentation explicitly says observe() can be used to keep the value of a computed:

you can always forcefully keep a computed value awake if you need to, by using either observe or keepAlive

Here: https://mobx.js.org/refguide/computed-decorator.html
Am I doing it wrong or is the documentation wrong?

Second

I like your workaround and I do not see any strong reason not to add it to the code base. The current implementation is broken; a computed does not have to be calculated before it is ever used.

Does any of this make sense?

[urugator]

observe() doesn't seem to keep the computed value cached:

You're observing stuff instead of coolStuff in your fiddle.
It does cache the value, but it causes it to immediately recalculate (same as autorun), because it actually uses autorun underneath, see here.

Am I doing it wrong or is the documentation wrong?

The value of computed is cached if it's observed by any autorun/reaction. So in all cases the idea is the same... create artifical autorun/reaction, which does nothing, but subscribes for that computed...

[hector]

@urugator Thanks for your help, it has been clarifying.

Is there any chance a PR introducing your workaround as the standard behaviour for keepAlive = true would be accepted?

Besides that, let me explain my use case:

class Book {
  // contains a url to a document with the full text of the book
  @observable url: string;

  // If this.url gets changed, this.text needs to be re-fetched
  @computed get text(): Promise<?string> {
    const url = this.url;
    return (async () => {
      if (!url) return;
      const response = await fetch(url);
      const text = await response.text();
      return text;
    })();
  }
}

// I retrieve all the books here, but I do not want to fetch the text of all of them
// unless I am really gonna use it
const books = getBooksFromDatabaseOrAPI();

// I use the text of one book, so it gets retrieved
books[0].text.then(text => doSomething(text));

// Some time later, I might want to access the text again without a re-fetch
books[0].text.then(text => doSomethingElse(text));

I know I can make it work with the workaround you proposed, but it is a lot of boilerplate and I am under the impression @computed({keepAlive: true}) should behave exactly as I need.

[mweststrate]

I think starting the keep alive semantics only at the first read would be a good improvement, so PR is welcome Op wo 9 mei 2018 15:51 schreef Hector Parra <[email protected]>:

…

@urugator <https://github.com/urugator> Thanks for your help, it has been clarifying. Is there any chance a PR introducing your workaround as the standard behaviour for keepAlive = true would be accepted? Besides that, let me explain my use case: class Book { // contains a url to a document with the full text of the book @observable url: string; // If this.url gets changed, this.text needs to be re-fetched @computed get text(): Promise<?string> { const url = this.url; return (async () => { if (!url) return; const response = await fetch(url); const text = await response.text(); return text; })(); } } // I retrieve all the books here, but I do not want to fetch the text of all of them// unless I am really gonna use itconst books = getBooksFromDatabaseOrAPI(); // I use the text of one book, so it gets retrieved books[0].text.then(text => doSomething(text)); // Some time later, I might want to access the text again without a re-fetch books[0].text.then(text => doSomethingElse(text)); I know I can make it work with the workaround you proposed, but it is a lot of boilerplate and I am under the impression @computed({keepAlive: true}) should behave exactly as I need. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1534 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhKKbWaunG3FvXgBHOD74e8Wbycovks5twvRTgaJpZM4T144U> .

[mweststrate]

And if you get stuck somewhere or it gets dirty you can also limit the PR to extensive unit test. P.s. don't forget the downside of keeping computeds alive; they mightbe introduce memory leaks Op do 10 mei 2018 09:05 schreef Michel Weststrate <[email protected]>:

…

I think starting the keep alive semantics only at the first read would be a good improvement, so PR is welcome Op wo 9 mei 2018 15:51 schreef Hector Parra ***@***.***>: > @urugator <https://github.com/urugator> Thanks for your help, it has > been clarifying. > > Is there any chance a PR introducing your workaround as the standard > behaviour for keepAlive = true would be accepted? > > Besides that, let me explain my use case: > > class Book { > // contains a url to a document with the full text of the book > @observable url: string; > > // If this.url gets changed, this.text needs to be re-fetched > @computed get text(): Promise<?string> { > const url = this.url; > return (async () => { > if (!url) return; > const response = await fetch(url); > const text = await response.text(); > return text; > })(); > } > } > // I retrieve all the books here, but I do not want to fetch the text of all of them// unless I am really gonna use itconst books = getBooksFromDatabaseOrAPI(); > // I use the text of one book, so it gets retrieved > books[0].text.then(text => doSomething(text)); > // Some time later, I might want to access the text again without a re-fetch > books[0].text.then(text => doSomethingElse(text)); > > I know I can make it work with the workaround you proposed, but it is a > lot of boilerplate and I am under the impression @computed({keepAlive: > true}) should behave exactly as I need. > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > <#1534 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ABvGhKKbWaunG3FvXgBHOD74e8Wbycovks5twvRTgaJpZM4T144U> > . >

[hector]

I submitted a PR which should solve the issue. I did not get to fully understand all the code base, so there may be better way to do it. I am open to suggestions :)

[tylerlong]

The current behavior is ridiculous.

computed values are cached when they are observed

I think it's safe to say "Computed values are not cached when they are not observed."

And it seems that the recommended way to keep a computed values observed is autorun. (keepAlive uses autorununderneath)

What are the points of cache?

1. compute as fewer times as possible
2. use cached value as much as possible

Here is the dilemma:

• If you don't turn on autorun, there is no cache for you. You cannot have point 2.
• If you do turn on autorun, then it re-computes every time dependencies change. You can not have point 1.

What a pity! Cannot user have both 1 & 2? IMHO, 1 & 2 are almost about the same thing. But MobX by design makes them contradictory.

[mweststrate]

if you do turn on autorun, then it re-computes every time that dependencies change. You can not have point 1.

That is the whole point; invalidate caches precisely when needed. A cache without invalidation is not a cache, but a copy. In other words: if a change in a dependency doesn't require a computed to be updated, then it is not a dependency in the first place! So recomputing on dependency change is the whole point of the library

The reason that they are not caching by default is that it simply creates a continues memory leak. If you read a computed value, expect it to subscribe to it's dependencies, then you are also responsible for unsubscribing once you are not interested in updates anymore, and that is the whole point of the reactions concept.

Please read the docs, background blogs or the MobX book with attent, in which this model is extensively described.

For any further discussion, please open a new issue, so that others can see the questions as well.

[urugator]

@mweststrate I think that @tylerlong has the point in case of keepAlive.
The thing is that keepAlive actually doesn't need the "current" value for anything. It just has to keep the old cached value and an information whether it's dirty. It shouldn't be recomputed at every change, but only when actually requested by something later (if dirty). (Considering that keepAlive is only "observer" at that time obviously)
I think that exactly the same problem is pointed out in this issue: mobxjs/mobx-utils#136

[mweststrate]

Right. Let's continue discussion on that one. Note that it is was reactions are normally doing already; they act as scheduler for computed values; computed values normally evaluate on next read by a reaction after dirty, not immediately. So I am curious about actual use cases where many reads are made to a computed over time, but without a reactive context. Op vr 19 okt. 2018 17:31 schreef urugator <[email protected]>:

…

@mweststrate <https://github.com/mweststrate> I think that @tylerlong <https://github.com/tylerlong> has the point in case of keepAlive. The thing is that keepAlive actually doesn't need the "current" value for anything. It just has to keep the old cached value and an information whether it's dirty. It shouldn't be recomputed at every change, but only when actually requested by something later. I think that exactly the same problem is pointed out in this issue: mobxjs/mobx-utils#136 <mobxjs/mobx-utils#136> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1534 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhNyGm3p2qnitC9K_lz3pOU_erEG2ks5umfA4gaJpZM4T144U> .

[urugator]

So I am curious about actual use cases where many reads are made to a computed over time, but without a reactive context.

That's not the case. The idea is that you have an expensive computation, whose value should "survive", the unsubscription of all observers (reactions) - stay cached until the value is requested again - by new observers or by invoking computed directly.
Currently you can use keepAlive to do that, but because the keepalive is just another reaction it will force reevaluation of computed at every dependency change.
This is a waste, because the computed value actually isn't important to this keepAlive kind of reaction - the value isn't consumed by anything, it doesn't have to notify anything, the value change can't cause any staleness, therefore there is no need to recompute it.
The computed just need to know it's in dirty state and should recompute after it is requested by normal reaction/invoked directly or return the cached value when it isn't dirty.

[mweststrate]

I think the behavior could be achieved by customizing the scheduler option of that autorun. Anybody interested to PR some unit tests that would verify the implementation of this smarter implementation?

[hector]

I think the behavior could be achieved by customizing the scheduler option of that autorun. Anybody interested to PR some unit tests that would verify the implementation of this smarter implementation?

I can write the unit tests.
Besides, I think that right now the 2 tests for keepAlive are validating multiple things. It could be nice to have every requirement isolated in its own test, so when they fail you know exactly what requirement is missing. Is that okay?

[mweststrate]

Sure! Op ma 22 okt. 2018 om 18:30 schreef Hector Parra <[email protected]>:

…

I think the behavior could be achieved by customizing the scheduler option of that autorun. Anybody interested to PR some unit tests that would verify the implementation of this smarter implementation? I can write the unit tests. Besides, I think that right now the 2 tests for keepAlive are validating multiple things. It could be nice to have every requirement isolated in its own test, so when they fail you know exactly what requirement is missing. Is that okay? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1534 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvGhKOQTTYhTVwY6KO3kZe0NsAkjLCMks5unfKugaJpZM4T144U> .

[hector]

Tests added!
I don't know how to implement the solution but, conceptually, I am quite sure we need to get rid of the autorun created for keepAlive and probably replace it with a cachedValue + dirty state.
Logically, the cachedValue never gets deleted (because keepAlive is true), cachedValue lives inside the observable and cachedValue will only disappear when the observable is disposed.

My application would benefit from these improvements, so I can confirm there are real world scenarios for it.

Besides that, I would argue that computed properties, in this library, are cached by default. So, in my understanding, it is contradictory that cache is on when computeds are observed but cache is off when they are not observed. To be homogeneous, keepAlive should be true by default. But whatever, it is just defaults.

[urugator]

keepAlive should be true by default

Mobx assumes that observables are consumed at some point by some reaction (otherwise there would be no need for observability).
When you dispose all consumers (reactions), the computed cache is disposed as well, otherwise there would be a possibility of leaking a memory.
The cache is used to prevent multiple subsequent recomputations inside single consumer (reaction) or multiple recomputations by multiple "concurrent" consumers (reactions) and to prevent notification when value didn't change (the cached value is needed for comparison).
keepAlive creates another consumer and therefore makes you responsible for potential memory leakage. keepAlive can't be slapped everywhere mindlessly.
So the default is "best effort" behavior - if it can safely optimize, it will, otherwise it won't.

[hector]

Mobx assumes that observables are consumed at some point by some reaction (otherwise there would be no need for observability)

You are right but I have the impression that "consuming" a computed property has a broader sense, in other words, you can consume a computed property by:

1. Subscribing to it, for example, to render it on a webpage
2. Access to it without subscribing

To give an example for case 2:

class Person {
  constructor(fullname) {
    this.fullname = fullname;
  }
  @observable fullname;
  @computed get firstname() {
    return this.fullname.split(' ')[0];
  }
}

const person = new Person('John Smith');
person.firstname // 'John'
runInAction(() => person.fullname = 'Kate Ryan');
person.firstname // 'Kate'

I don't want to have to subscribe to firstname to consume it.
Besides, a great deal of my computed properties are consumed by method 1 and 2. When I access a computed property somewhere in my code it always has to be valid and it always has to avoid unnecessary computations. I don't care if other consumers are subscribed to it or not, and I shouldn't. Because I don't care how are subscriptions to a regular observable property when I access it right?

When you dispose all consumers (reactions), the computed cache is disposed as well, otherwise there would be a possibility of leaking a memory.

Dispose the cache when the computed property is disposed, problem solved.
For observables we are not disposing its value when there are no more subscriptions.
In my example:

const person = null; // now observables, computeds and its caches will be disposed 

keepAlive creates another consumer and therefore makes you responsible for potential memory leakage. keepAlive can't be slapped everywhere mindlessly.

keepAlive should not create another consumer. This is a bad solution, and I know I wrote it myself :D

[hector]

Given my previous example of Person, I would implement the proposed computed property with something similar to the following code. Take it as pseudocode.

class Person {
  @observable fullname;

  @observable computedCache; // you can subscribe with this
  computedCacheIsDirty = true;

  constructor(fullname) {
    this.fullname = fullname;
    reaction(
      () => this.fullname,
      fullname => {
        this.computedCacheIsDirty = true;
        if (computedHasConsumers()) { // this is what I don't know how to do
          // If computed has consumers we do the calculations right away
          this.firstname
        }
      }
    );
    // I don't know how to do that either:
    // Detect the first consumer of the computed and do the calculations before sending 
    // the computed value
    detectNewConsumer(consumersNumber => (consumersNumber === 1) && this.firstname);
  }

  // read the computed from here
  get firstname() {
    if (this.computedCacheIsDirty) this.computedCalculation();
    return this.computedCache;
  }

  @action computedCalculation() {
    this.computedCache = this.userFunctionForComputed();
    this.computedCacheIsDirty = false;
  }

  userFunctionForComputed() {
    // This is the thing the developer will write inside the computed function
    return this.fullname.split(' ')[0];
  }
}

[spion]

FYI this is what I ended up using for a non-auto-recomputing keepAlive:

#1332 (comment)

[urugator]

For observables we are not disposing its value when there are no more subscriptions.

Please someone correct me if I am wrong: The problem isn't the cached value itself the problem is invalidation of the value. The subscriptions have to be maintained between computed and it's deps, which prevents them from being garbage collected (unless all refs are lost).
Not sure if WeakSet could be used to manage these subscriptions.

[mweststrate]

The new behavior is implemented in 4.6.0 / 5.6.0!

[hector]

Thank you @mweststrate!!

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.