unintuitive behavior when an observable is created and then observed within the same computed

[melnikov-s]

I have a:

2. Issue:

• Are you willing to (attempt) a PR yourself?

Within a computed function you're allowed to create an observable and then modify it. This makes perfect sense and was resolved in issue #563. Yet if you then proceed to read from the observable you just created within the computed function, it will be observed. Which can lead to some surprising and unintuitive behavior.

Consider the following example:

class Foo {
  @computed
  get store() {
    return new Store({ name: 'foo' });
  }
}
class Store {
 @observable obj;
 @observable name;

 constructor(obj) {
  this.obj = obj;
  this.name = obj.name;
 }


}

const f = new Foo();
autorun(() => f.store);
const g = f.store;
console.log(g === f.store)
g.obj.name = 'new name';
console.log(g === f.store)

This will log

true
true

but within the constructor if you change this.name = obj.name; to this.name = this.obj.name you would expect the same behavior but instead you will now get:

true
false

since this.obj is now observed by the computed even though the this was created within that same computed.

The proposal here would be to make created observables completely silent for both read and write operations to the computed function that created them.

This will avoid the behaviour outlined above but also avoid a needless re-computation since changing an observable produced by a computed will not change the value that the computed produces.

Example:

let o1;
const c = computed(() => {
  const o2 = observable.box(2);

  o2.set(o2.get() * 2);

  return o2;
});

autorun(() => {
   o1 = c.get();
  console.log(o1.get())
})
// logs 4
o1.set(10)
// re-evaluates but still logs 4

It is possible to produce a different value but that would involve modifying outside state within the computed making it an anti-pattern.

[danielkcz]

I wonder, where did you actually get the idea that creating observable in computed is a good thing? There is no such recommendation in the linked issue. From my limited understanding, the computed is deemed to rerun and your observable is recreated from scratch and throwing away the previous state.

Also, note that issue is 3 years old and talking about MobX3 which is no longer maintained. I don't know what exactly changed there, but I wouldn't recommend getting inspiration from such an old issue.

[melnikov-s]

The linked issue was specifically about allowing to create observables within a computed and not produce a strict violation.

As a result this pr #812 was produced which explicitly enabled this behavior (In Mobx 3 yes but it's true for 4/5 as well).

It is allowed to create and modify observable values in computed blocks, as long as they are not observed yet.

---

I wonder, where did you actually get the idea that creating observable in computed is a good thing?

Why would creating observable values in computed be not a good thing in your opinion? Observable values are values after all and that's what a computed is supposed to produce. We currently use this pattern in production with the only caveat being this particular issue.

[danielkcz]

Why would creating observable values in computed be not a good thing in your opinion?

Not sure just doesn't feel right 😆But hey, I am using like half of MobX features, so don't listen to me.

I suppose it cannot hurt if you proceed with PR. Be sure to add tests. Start at the master branch and when it turns out ok, we will need another PR to mobx4-master.

[urugator]

At first I wanted to offer configure({ enforceActions: "observed" }) which would force you to wrap the constructor inside action, fixing the problem.
However if we modify the code like this:

configure({ enforceActions: "observed" });

class Foo {
  @computed
  get store() {
    return new Store({ name: 'foo' });
  }
}
class Store {
 @observable obj;
 @observable name;

 constructor(obj) {  
  this.obj = obj;
  const name = this.obj.name; // subscribed to obj.name
  // statisfy enforceActions
  runInAction(() => {
    this.name = name;
  });
 }


}

const f = new Foo();
autorun(() => f.store);
const g = f.store;
console.log(g === f.store) // true
// statisfy enforceActions
runInAction(() => {
  g.obj.name = 'new name';
})
console.log(g === f.store) // false

It will also yield true false without throwing.

I think there are some problems with the suggested solution:

1. Subscribing to computed (which is also sort of observable) inside derivation is valid use case:

autorun(() => {
  computed(() => o.x + o.y).get();
})

I know this is unaffected by the PR, but raises some questions about how it plays together.

2. derivation.runId !== observable.computedCreatedIn probably isn't sufficient heuristic, because you can "leak" the observable to some other computed, leading to the same problem.

const cmp = computed(() => {  
  const o = computed(() => {
    // create observable inside nested derivation
    const o = observable({ x: 1 });
    o.x; // subscribe (fine)
    return o;
  }).get();  
  o.x; // subscribe (derivation id mismatch)  
  return o;
})
// then somewhere
const o = cmp.get();
console.log(o === cmp.get()) // true
o.x = 2;
console.log(o === cmp.get()) // false

So perhaps we would need a reactionId, so that any observable created inside that reaction would be excluded from subscription, not sure about the consequences...

Why would creating observable values in computed be not a good thing in your opinion?

There are 2 associated problems:

1. Returning values that are not comparable by value (array/object) - computed is invalidated, structure is recreated, looks same, but fails on comparison -> associated state (if any) is lost and observers are unnecessarily notified.
2. Returning stateful objects - requires keepAlive so that the state is not lost inbetween subscriptions/unsubscriptions.

[melnikov-s]

const cmp = computed(() => {  
  const o = computed(() => {
    // create observable inside nested derivation
    const o = observable({ x: 1 });
    o.x; // subscribe (fine)
    return o;
  }).get();  
  o.x; // subscribe (derivation id mismatch)  
  return o;
})
// then somewhere
const o = cmp.get();
console.log(o === cmp.get()) // true
o.x = 2;
console.log(o === cmp.get()) // false

[Assuming you meant keepAlive: true otherwise the first comparison will be false as well] That is actually not the behavior with my PR. It returns true for both which is wrong. runId does not appear to increment in this situation like I thought it would, rather a combination of runId and computationDepth seems to do the trick. I do indeed want true, false for the same reason as the following:

const cmp = computed(() => {  
  const o = computed(() => {
    const o = observable({ x: 1 });
    o.x;
    o.x = 5; // no problem
    return o;
  }).get();  
  o.x; 
  o.x = 10; // will throw
  return o;
})

Essentially what I would like is to have both the read and write be silent for observables that are created within a computed for that computed only (currently only write is). Not sure what the best way to achieve that is, using something like ${runId}:${computationDepth} appears to work but also comes off as a bit hacky. What's a better approach here?

There are 2 associated problems:

Yeah but these issues are not unique to observables but rather to non primitive values, so keepAlive and struct or custom equals need to be used if needed. We don't use observable.box in our production app but make heavy use of creating computed sub-stores that have observable properties on them. Losing that state when when a dependency of the computed changes is actually what we want.

For example we might generate a computed store that's based on some observable selection, that store itself will have a bunch of observable state related to that selection and when that selection changes we want to generate a new store and reset all state associated with the previous selection.

More concretely, say we have a list of items on the right and a detail view on the left, when a user selects an item we want to generate a details store which also has some state on it. When they select another item, we want to dispose of the previous details store and create a new one based on the new selection. We achieve this using a computed method which generates a new store.

class MyRootStore {
    @observable selection = null;
    @computed get detailStore() {
         if (this.selection) {
            return new DetailStore(this.selection);
         }

         return null;
    }
}

Hope that makes sense.

[urugator]

[Assuming you meant keepAlive: true

I am assuming there is some observer, same situation as above.
The idea is, that in theory it's possible to nest these constructors and computeds in a way that you would unintentionally subscribe the newly created observable inside different computed than the one which created it.

It returns true for both which is wrong.

Interesting. I would expect it to go somehow like:
o created in nestedComputed
nestedComputeId assigned to o.computedCreatedIn
o.x accessed in nestedComputed and ignored because o.computedCreatedIn === nestedComputedId
o.x accessed in outerComputed and subscribed because o.computedCreatedIn !== outerComputedId
o.x modified -> outerComputed recomputing -> recomputing nestedComputed -> creating new o.x -> returning new o.x

I briefly checked that globalState.runId is different inside each computation (and each invocation) is it not?

Why we want it to yield true, false?
I can't imagine how a modification of o.x could affect the result of the whole derivation, so why it should re-run?

Hope that makes sense.

It does, I used to do it quite often, but didn't work well for me in the end.

[melnikov-s]

I briefly checked that globalState.runId is different inside each computation (and each invocation) is it not?

My mistake, I was using keepAlive on the outer computed which was causing true, true, wrapping it in a reaction produces the expected true, false

Why we want it to yield true, false?
I can't imagine how a modification of o.x could affect the result of the whole derivation, so why it should re-run?

I don't know to be honest, I suppose for the same reason why you can write to the observable in the inner computed but it throws in the outer. Perhaps ideally it does not re-run and writing to the observable from the outer computed does not throw. I'm thinking the behavior for both read and write should be consistent here either way.

You're right, we want true, true. It seems like a computed created and evaluated within another computed could essentially be inlined and therefore the inner and outer computed functions should behave the same for both read and write.

[urugator]

Hm, but disabling subscription for the whole reaction is a no-no:

@computed
get baz() {       
  const store = this.foo.store; // creates new store (therefore all it's props are excluded from subscription inside current reaction)    
  // still inside same reaction
  return store.name; // not subscribed!     
}

It makes sense only if the whole graph is created inside reaction:

@computed
get baz() {   
  const foo = new Foo();    
  const store = foo.store; // creates new store (therefore all it's props are excluded from subscription inside current reaction)    
  // still inside same reaction
  return store.name; // not subscribed - but in this case it's correct, because store.name will always yield the same value     
}

[melnikov-s]

It makes sense only if the whole graph is created inside reaction:

Yup, as long as the computed is created and evaluated within another the proposed behavior applies. When I get some time I'll update the PR with what we discussed.

[urugator]

Just for completeness - it applies to reaction as well. The difference is that in computed the value is returned, while in reaction the value is used for side effect:

render() {
  const store = this.props.foo.store;      
  return store.name; // should susbcribe
}

vs whole graph created inside derivation

render() {      
  const foo = new Foo();    
  const store = foo.store;      
  return store.name; // shouldn't subscribe   
}

[melnikov-s]

Hm, what about

let o;
let x = observable.box(1);

autorun(() => {
  o = o || observable.box(1);
  x.get();
  o.get(); 
});

o.set(2) // don't re-run
x.set(2) // re-run
o.set(3) // re-run ?

Gets a bit trickier with reactions as the above pattern is valid (?) while it's an anti-pattern with computed.

[urugator]

On the second run (x.set(2)) the o is not created in derivation, so runId should mismatch (during reportObserved) and therefore subscribe and correctly re-run on o.set(3)?
However I am afraid we won't be able to reasonably detect all these scenarios correctly. Really not sure. There seems to be quite a lot of ways to leak the observable outside/inside of derivation and mess things up. Even calling action from computed isn't strictly forbidden (or is it?)

[melnikov-s]

Yeah not to mention that if we apply this to reactions, things like ObservableMap.prototype.has will break as it has the side effect of creating a new atom which we definitely want observed.

Maybe the PR is ok as-is , it will re-evaluate needlessly on nested computes but that's not a change in behavior and is consistent with how write works while fixing the common case of creating observables in computed functions.

[urugator]

ObservableMap.prototype.has will break as it has the side effect of creating a new atom which we definitely want observed.

Isn't it the same thing? If I call map.has inside computed it won't subscribe for that atom... ?

[melnikov-s]

Isn't it the same thing? If I call map.has inside computed it won't subscribe for that atom... ?

Yeah, I somehow missed that since no computed-related tests broke with my PR.

Alright it seems like it's a no-go then and this issue and PR can be closed as we have no way to track whether or not an observable gets leaked outside of the computed/reaction.

Thanks for your input on this.

I can modify that PR / create a new one to add a unit test for the use ObservableMap.prototype.has in a computed in order to capture that behavior if you think that'll be useful.

[melnikov-s]

To summarize:

If an observable is created within a derivation we do not want to subscribe to it if the result of re-running the derivation will just end up re-creating the observable. As this will not produce a new value when the derivation is a computed and might lead to surprising behavior as documented in the initial issue comment.

Yet a valid use case might be to create an observable lazily within a derivation and cache it for remaining runs. In this case we do want the derivation to re-run when the observable changes. Otherwise the derivation will be stale.

Problem is that there's no way for mobx tell within a derivation whether or not the created observable will be re-recreated each time or just lazily initialized. It would have to be up to the consumer to be aware of this issue and use an api to flag it.

Currently all observables created within a derivation behave as though they are lazily initialized and the consumer would have to use untracked to opt-out. Alternatively all observables created within a derivation can be treated as non-lazy and the consumer would need a new api like{ lazy: true } when creating observables to signal otherwise. I think the later is the more common use-case but introducing new api and behavior breaking changes is a hard sell.