Fine tuned: error: only throw when an observed observable is being ch…

…anged

CHANGELOG.md

@@ -1,6 +1,8 @@
 # 3.0.3
 
-* Fixed #798: MobX will not throw when invoking an action for computed properties. This is still an anti pattern in general, but caused to often issues when creating new objects in a computed property (often, but not always anti pattern as well) that made use of it's own internal actions
+Fixed #798: MobX will not throw when invoking an action during the evaluation of a computed property.
+This is still an anti pattern in general, but caused to often issues when creating new objects in a computed property (often, but not always anti pattern as well) that made use of it's own internal actions.
+MobX will however still throw if an attempt to change an observable that is already being observed is being changed.
 
 # 3.0.2
 

src/core/computedvalue.ts

@@ -141,6 +141,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 
 	computeValue(track: boolean) {
 		this.isComputing = true;
+		globalState.computationDepth++;
 		const prevAllowStateChanges = allowStateChangesStart(false);
 		let res: T | CaughtException;
 		if (track) {
@@ -153,6 +154,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 			}
 		}
 		allowStateChangesEnd(prevAllowStateChanges);
+		globalState.computationDepth--;
 		this.isComputing = false;
 		return res;
 	};
@@ -201,7 +203,7 @@ WhyRun? computation '${this.name}':
 ` * This computation will re-run if any of the following observables changes:
     ${joinStrings(observing)}
     ${(this.isComputing && isTracking) ? " (... or any observable accessed during the remainder of the current run)" : ""}
-	${getMessage("m038")} 
+	${getMessage("m038")}
 
   * If the outcome of this computation changes, the following observers will be re-run:
     ${joinStrings(observers)}

src/core/derivation.ts

@@ -1,6 +1,6 @@
 import {IObservable, IDepTreeNode, addObserver, removeObserver} from "./observable";
 import {globalState} from "./globalstate";
-import {invariant} from "../utils/utils";
+import {fail} from "../utils/utils";
 import {isComputedValue} from "./computedvalue";
 import {getMessage} from "../utils/messages";
 
@@ -102,12 +102,18 @@ export function isComputingDerivation() {
 	return globalState.trackingDerivation !== null; // filter out actions inside computations
 }
 
-export function checkIfStateModificationsAreAllowed() {
-	if (!globalState.allowStateChanges) {
-		invariant(false, globalState.strictMode
-			? getMessage("m030")
-			: getMessage("m031")
-		);
+export function checkIfStateModificationsAreAllowed(atom: IObservable) {
+	if (globalState.allowStateChanges === false) {
+		if (globalState.strictMode)
+			fail(getMessage("m030") + atom.name);
+		else
+			fail(getMessage("m031") + atom.name);
+	} else {
+		// Observed observables should not be modified during a computation,
+		// even not from inside an action!
+		if (globalState.computationDepth > 0 && atom.observers.length > 0) {
+			fail(getMessage("m027") + atom.name);
+		}
 	}
 }
 

src/core/globalstate.ts

@@ -21,6 +21,11 @@ export class MobXGlobals {
 	 */
 	trackingDerivation: IDerivation | null = null;
 
+	/**
+	 * Are we running a computation currently? (not a reaction)
+	 */
+	computationDepth = 0;
+
 	/**
 	 * Each time a derivation is tracked, it is assigned a unique run-id
 	 */

src/types/observablearray.ts

@@ -134,7 +134,7 @@ class ObservableArrayAdministration<T> implements IInterceptable<IArrayWillChang
 	}
 
 	spliceWithArray(index: number, deleteCount?: number, newItems?: T[]): T[] {
-		checkIfStateModificationsAreAllowed();
+		checkIfStateModificationsAreAllowed(this.atom);
 		const length = this.values.length;
 
 		if (index === undefined)
@@ -481,7 +481,7 @@ function createArraySetter(index: number) {
 		const values = adm.values;
 		if (index < values.length) {
 			// update at index in range
-			checkIfStateModificationsAreAllowed();
+			checkIfStateModificationsAreAllowed(adm.atom);
 			const oldValue = values[index];
 			if (hasInterceptors(adm)) {
 				const change = interceptChange<IArrayWillChange<T>>(adm as any, {

src/types/observablevalue.ts

@@ -63,7 +63,7 @@ export class ObservableValue<T> extends BaseAtom implements IObservableValue<T>,
 	}
 
 	private prepareNewValue(newValue): T | IUNCHANGED {
-		checkIfStateModificationsAreAllowed();
+		checkIfStateModificationsAreAllowed(this);
 		if (hasInterceptors(this)) {
 			const change = interceptChange<IValueWillChange<T>>(this, { object: this, type: "update", newValue });
 			if (!change)

src/utils/messages.ts

@@ -25,10 +25,11 @@ const messages = {
 "m024" : "whyRun() can only be used if a derivation is active, or by passing an computed value / reaction explicitly. If you invoked whyRun from inside a computation; the computation is currently suspended but re-evaluating because somebody requested its value." ,
 "m025" : "whyRun can only be used on reactions and computed values" ,
 "m026" : "`action` can only be invoked on functions" ,
+"m027" : "Computed values are not allowed to not cause side effects by changing observables that are already being observed. Tried to change: ",
 "m028" : "It is not allowed to set `useStrict` when a derivation is running" ,
 "m029" : "INTERNAL ERROR only onBecomeUnobserved shouldn't be called twice in a row" ,
-"m030" : "It is not allowed to create or change state outside an `action` when MobX is in strict mode. Wrap the current method in `action` if this state change is intended" ,
-"m031" : "It is not allowed to change the state when a computed value or transformer is being evaluated. Use 'autorun' to create reactive functions with side-effects." ,
+"m030" : "It is not allowed to create or change state outside an `action` when MobX is in strict mode. Wrap the current method in `action` if this state change is intended. Tried to change: " ,
+"m031" : "It is not allowed to change the state when a computed value or transformer is being evaluated. Use 'autorun' to create reactive functions with side-effects. Tried to change: " ,
 "m032" : "* This computation is suspended (not in use by any reaction) and won't run automatically.\n	Didn't expect this computation to be suspended at this point?\n	  1. Make sure this computation is used by a reaction (reaction, autorun, observer).\n	  2. Check whether you are using this computation synchronously (in the same stack as they reaction that needs it).",
 "m033" : "`observe` doesn't support the fire immediately property for observable maps." ,
 "m034" : "`mobx.map` is deprecated, use `new ObservableMap` or `mobx.observable.map` instead" ,

test/action.js

@@ -172,21 +172,49 @@ test('should be possible to create autorun in ation', t => {
 	t.end();
 })
 
-test.skip('should not be possible to invoke action in a computed block', t => {
-	// Disabled as of #798
+test('should be possible to change unobserved state in an action called from computed', t => {
 	var a = mobx.observable(2);
 
-	var noopAction = mobx.action(() => {});
+	var testAction = mobx.action(() => {
+		a.set(3)
+	});
+
+	var c = mobx.computed(() => {
+		testAction();
+	});
+
+	t.plan(1)
+	mobx.autorun(() => {
+		t.doesNotThrow(() => {
+			c.get()
+		})
+	});
+
+	mobx.extras.resetGlobalState();
+	t.end();
+});
+
+test('should not be possible to change observed state in an action called from computed', t => {
+	var a = mobx.observable(2);
+	var d = mobx.autorun(() => {
+		a.get()
+	})
+
+	var testAction = mobx.action(() => {
+		a.set(3)
+	});
 
 	var c = mobx.computed(() => {
-		noopAction();
-		return a.get();
+		testAction();
+		return a.get()
 	});
 
-	utils.consoleError(t, () => {
-		mobx.autorun(() => c.get());
-	}, /Computed values or transformers should not invoke actions or trigger other side effects/, 'expected throw');
+	t.throws(() => {
+		c.get()
+	}, /Computed values are not allowed to not cause side effects by changing observables that are already being observed/)
+
 	mobx.extras.resetGlobalState();
+	d();
 	t.end();
 });