Merge pull request #812 from mobxjs/fix/798-invoke-action-from-computed

Relax strict mode, solves #798, #563

CHANGELOG.md

@@ -1,3 +1,28 @@
+# 3.1.0
+
+** Improved strict mode **
+
+Strict mode has been relaxed a bit in this release. Also computed values can now better handle creating new observables (in an action if needed). The semantics are now as follows:
+
+* In strict mode, it is not allowed to modify state that is already being _observed_ by some reaction.
+* It is allowed to create and modify observable values in computed blocks, as long as they are not _observed_ yet.
+
+In order words: Observables that are not in use anywhere yet, are not protected by MobX strict mode.
+This is fine as the main goal of strict mode is to avoid kicking of reactions at undesired places.
+Also strict mode enforces batched mutations of observables (through action).
+However, for unobserved observables this is not relevant; they won't kick of reactions at all.
+
+This fixes some uses cases where one now have to jump through hoops like:
+* Creating observables in computed properties was fine already, but threw if this was done with the aid of an action. See issue [#798](https://github.com/mobxjs/mobx/issues/798).
+* In strict mode, it was not possible to _update_ observable values without wrapping the code in `runInAction` or `action`. See issue [#563](https://github.com/mobxjs/mobx/issues/563)
+
+Note that the following constructions are still anti patterns, although MobX won't throw anymore on them:
+* Changing unobserved, but not just created observables in a computed value
+* Invoke actions in computed values. Use reactions like `autorun` or `reaction` instead.
+
+Note that observables that are not in use by a reaction, but that have `.observe` listeners attached, do *not* count towards being observed.
+Observe and intercept callbacks are concepts that do not relate to strict mode, actions or transactions.
+
 # 3.0.2
 
 * Fixed issue where MobX failed on environments where `Map` is not defined, #779 by @dirtyrolf

package.json

@@ -1,6 +1,6 @@
 {
   "name": "mobx",
-  "version": "3.0.2",
+  "version": "3.0.2-fix798-2",
   "description": "Simple, scalable state management.",
   "main": "lib/mobx.js",
   "typings": "lib/mobx.d.ts",
@@ -77,4 +77,4 @@
     "state management",
     "data flow"
   ]
-}
+}

src/api/observabledecorator.ts

@@ -1,4 +1,3 @@
-import {allowStateChangesStart, allowStateChangesEnd} from "../core/action";
 import {asObservableObject, defineObservableProperty, setPropertyValue} from "../types/observableobject";
 import {invariant, assertPropertyConfigurable} from "../utils/utils";
 import {createClassPropertyDecorator} from "../utils/decorators";
@@ -13,11 +12,8 @@ export function createDecoratorForEnhancer(enhancer: IEnhancer<any>) {
 			assertPropertyConfigurable(target, name);
 			invariant(!baseDescriptor || !baseDescriptor.get, getMessage("m022"));
 
-			// might happen lazily (on first read), so temporarily allow state changes..
-			const prevA = allowStateChangesStart(true);
 			const adm = asObservableObject(target, undefined);
 			defineObservableProperty(adm, name, baseValue, enhancer);
-			allowStateChangesEnd(prevA);
 		},
 		function (name) {
 			const observable = this.$mobx.values[name];

src/core/action.ts

@@ -3,7 +3,6 @@ import {invariant} from "../utils/utils";
 import {untrackedStart, untrackedEnd} from "../core/derivation";
 import {startBatch, endBatch} from "../core/observable";
 import {isSpyEnabled, spyReportStart, spyReportEnd} from "../core/spy";
-import {isComputedValue} from "../core/computedvalue";
 import {globalState} from "../core/globalstate";
 import {getMessage} from "../utils/messages";
 
@@ -41,9 +40,6 @@ interface IActionRunInfo {
 }
 
 function startAction(actionName: string, fn: Function, scope: any, args?: IArguments): IActionRunInfo {
-		// actions should not be called from computeds. check only works if the computed is actively observed, but that is fine enough as heuristic
-	invariant(!isComputedValue(globalState.trackingDerivation), getMessage("m027"));
-
 	const notifySpy = isSpyEnabled() && !!actionName;
 	let startTime: number = 0;
 	if (notifySpy) {
@@ -90,9 +86,18 @@ export function isStrictModeEnabled(): boolean {
 }
 
 export function allowStateChanges<T>(allowStateChanges: boolean, func: () => T): T {
+	// TODO: deprecate / refactor this function in next major
+	// Currently only used by `@observer`
+	// Proposed change: remove first param, rename to `forbidStateChanges`,
+	// require error callback instead of the hardcoded error message now used
+	// Use `inAction` instead of allowStateChanges in derivation.ts to check strictMode
 	const prev = allowStateChangesStart(allowStateChanges);
-	const res = func();
-	allowStateChangesEnd(prev);
+	let res;
+	try {
+		res = func();
+	} finally {
+		allowStateChangesEnd(prev);
+	}
 	return res;
 }
 

src/core/computedvalue.ts

@@ -1,7 +1,7 @@
 import {IObservable, reportObserved, propagateMaybeChanged, propagateChangeConfirmed, startBatch, endBatch, getObservers} from "./observable";
 import {IDerivation, IDerivationState, trackDerivedFunction, clearObserving, untrackedStart, untrackedEnd, shouldCompute, CaughtException, isCaughtException} from "./derivation";
 import {globalState} from "./globalstate";
-import {allowStateChangesStart, allowStateChangesEnd, createAction} from "./action";
+import {createAction} from "./action";
 import {createInstanceofPredicate, getNextId, valueDidChange, invariant, Lambda, unique, joinStrings, primitiveSymbol, toPrimitive} from "../utils/utils";
 import {isSpyEnabled, spyReport} from "../core/spy";
 import {autorun} from "../api/autorun";
@@ -141,7 +141,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 
 	computeValue(track: boolean) {
 		this.isComputing = true;
-		const prevAllowStateChanges = allowStateChangesStart(false);
+		globalState.computationDepth++;
 		let res: T | CaughtException;
 		if (track) {
 			res = trackDerivedFunction(this, this.derivation, this.scope);
@@ -152,7 +152,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 				res = new CaughtException(e);
 			}
 		}
-		allowStateChangesEnd(prevAllowStateChanges);
+		globalState.computationDepth--;
 		this.isComputing = false;
 		return res;
 	};
@@ -201,7 +201,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,7 @@
 import {IObservable, IDepTreeNode, addObserver, removeObserver} from "./observable";
+import {IAtom} from "./atom";
 import {globalState} from "./globalstate";
-import {invariant} from "../utils/utils";
+import {fail} from "../utils/utils";
 import {isComputedValue} from "./computedvalue";
 import {getMessage} from "../utils/messages";
 
@@ -102,13 +103,14 @@ 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: IAtom) {
+	const hasObservers = atom.observers.length > 0;
+	// Should never be possible to change an observed observable from inside computed, see #798
+	if (globalState.computationDepth > 0 && hasObservers)
+		fail(getMessage("m031") + atom.name);
+	// Should not be possible to change observed state outside strict mode, except during initialization, see #563
+	if (!globalState.allowStateChanges && hasObservers)
+		fail(getMessage(globalState.strictMode ? "m030a" : "m030b") + 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/core/reaction.ts

@@ -222,9 +222,9 @@ function runReactionsHelper() {
 	// we converge to no remaining reactions after a while.
 	while (allReactions.length > 0) {
 		if (++iterations === MAX_REACTION_ITERATIONS) {
-			allReactions.splice(0); // clear reactions
 			console.error(`Reaction doesn't converge to a stable state after ${MAX_REACTION_ITERATIONS} iterations.`
 				+ ` Probably there is a cycle in the reactive function: ${allReactions[0]}`);
+			allReactions.splice(0); // clear reactions
 		}
 		let remainingReactions = allReactions.splice(0);
 		for (let i = 0, l = remainingReactions.length; i < l; i++)

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/observablemap.ts

@@ -1,6 +1,5 @@
 import {IEnhancer, deepEnhancer} from "./modifiers";
 import {untracked} from "../core/derivation";
-import {allowStateChanges} from "../core/action";
 import {IObservableArray, ObservableArray} from "./observablearray";
 import {ObservableValue, UNCHANGED} from "./observablevalue";
 import {createInstanceofPredicate, isPlainObject, getNextId, Lambda, invariant, deprecated, isES6Map, fail} from "../utils/utils";
@@ -66,9 +65,7 @@ export class ObservableMap<V> implements IInterceptable<IMapWillChange<V>>, ILis
 	changeListeners = null;
 
 	constructor(initialData?: IObservableMapInitialValues<V>, public enhancer: IEnhancer<V> = deepEnhancer, public name = "ObservableMap@" + getNextId()) {
-		allowStateChanges(true, () => {
-			this.merge(initialData);
-		});
+		this.merge(initialData);
 	}
 
 	private _has(key: string): boolean {

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,11 +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 or transformers should not invoke actions or trigger other side effects" ,
 "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." ,
+"m030a" : "Since strict-mode is enabled, changing observed observable values outside actions is not allowed. Please wrap the code in an `action` if this change is intended. Tried to modify: " ,
+"m030b" : "Side effects like changing state are not allowed at this point. Are you trying to modify state from, for example, the render function of a React component? Tried to modify: " ,
+"m031" : "Computed values are not allowed to not cause side effects by changing observables that are already being observed. Tried to modify: ",
 "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" ,
@@ -61,6 +61,6 @@ If that all doesn't help you out, feel free to open an issue https://github.com/
 `
 };
 
-export function getMessage(id: string) {
+export function getMessage(id: string): string {
 	return messages[id];
 }

test/action.js

@@ -172,20 +172,49 @@ test('should be possible to create autorun in ation', t => {
 	t.end();
 })
 
-test('should not be possible to invoke action in a computed block', t => {
+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();
 });
 

test/extras.js

@@ -160,27 +160,6 @@ test('spy 1', function(t) {
     t.end();
 })
 
-test('strict mode checks', function(t) {
-    var x = mobx.observable(3);
-
-    mobx.extras.allowStateChanges(false, function() {
-        x.get();
-    });
-
-    mobx.extras.allowStateChanges(true, function() {
-        x.set(7);
-    });
-
-    t.throws(function() {
-        mobx.extras.allowStateChanges(false, function() {
-            x.set(4);
-        });
-    });
-
-	mobx.extras.resetGlobalState();
-    t.end();
-});
-
 test('get atom', function(t) {
 	mobx.extras.resetGlobalState();
 	mobx.extras.getGlobalState().mobxGuid = 0; // hmm dangerous reset?