suggestion: log warning when computed property is accessed outside reaction/observer

[marty-wang]

One of my component got great performance downgrade after refactoring. It turns out that the computed property is accessed in a hot code path that is not in reaction/observer, therefore the computed property got recomputed over and over again.

The is one of those errors that is really hard to track down. It would be nice that mobx would log warning in the debug mode when the computed property is used this way. After all, computed properties are primarily supposed to be used in reaction/observer, which enables its memorization capability. However, because of the fact that you can still use computed property in non reaction/observer context, it makes really hard to track down the performance problem caused by it.

Please consider it, or provide other way to warn developer when they use computed properties in wrong context.

[urugator]

What about accessing computed inside "writing" context?

@computed get sum() {
  return this.a + this.b
}

@action doSomething() {  // not necessarily action/tx/batch?
  this.a = 5;
  console.log(this.sum) // do something with sum
}  

[marty-wang]

@urugator you can always extract the body of the computed property to a method, and use that method in other context. This way the expectation is clear, i.e. the computed is expected to be memorized and the extracted method is expected to re-compute every time, just like a normal method is supposed to do.

[urugator]

Being used in writing context doesn't mean it needs to recompute every time the context is invoked. It needs to be recomputed only if the dependencies were changed. That may happen conditionaly or not at all.
Also from the perspective of consumer, how do you recognize that something is computed, the semantic is indistinguishable from non-computed prop, which can be used anywhere...

[marty-wang]

@urugator

Before we discuss further, I hope we can agree that a good API should have the following properties

1. clear in semantic, meaning that it does exactly what it says it does, no more no less.
2. behaves consistently, not contextually, meaning that it does not work one way in some context and work another in other context
3. makes doing right thing easier and makes doing wrong thing harder
4. communicate to developers with helpful messages when they do something wrong.

If we cannot agree on these, I don't think we can be productive in the thread and might as well stop here.

So, back to the computed property, first, it clearly does not meet point 2, because it memoizes in reaction/observer but does not in other context, which makes bugs very hard to discover and developers have to constantly remind themselves of that. Refactoring can easily cause the bug. second, it is neutral to point 3 and third it does nothing for point 4.

I am not proposing for computed property to hit all these 4 points. but at lease it should warn developers that they might have done something wrong. Won't you want that when you use other people's API?

[urugator]

does not meet point 2

Throwing/warning outside of specific context is context depedent behavior as well.
Only context independent solution would be to always use keepAlive computed.

when they do something wrong

But is using computed inside non-tracking context wrong? To me it seem that the ability to switch between these two modes based on the context is actually considered a feature (I may be wrong of course).
I don't deny it would help in some cases, I am just suggesting it may cause problems in other (perhaps) valid use cases.

[mayorovp]

I suggest to log warining about perfomance when unobserved computed used outside of reaction or observer.

[Bnaya]

I want to extend the request for accessing any observable value, not only computed.
it will help to catch cases like in the common pitfalls:

React.render(<Timer timerData={timerData.secondsPassed} />, document.body)

The developer will see a warning about that.

[mweststrate]

MobX 4 will introduce mobx.configure({ warnOnUnsafeComputationReads: true })

[urugator]

@mweststrate Couldn't computed accept an requiresReaction flag? And have a global option like computedRequiresReaction = true.
The idea is to make this behavior configurable on computed level (to supress/enforce warning).

Also not fan of unsafe, what about warnOnComputedReadsOutsideReaction?

[mweststrate]

@urugator yes that sounds good! I was thinking on a computed level flag as well indeed, but couldn't figure any name that would fit in 80 chars linelength. requiresReaction is neat

[urugator]

I was also thinking about expensive flag, but requiresReaction seems more explanatory...