Improved strict mode as discussed in mobxjs#798

Author: mweststrate

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/core/derivation.ts

@@ -1,4 +1,5 @@
 import {IObservable, IDepTreeNode, addObserver, removeObserver} from "./observable";
+import {IAtom} from "./atom";
 import {globalState} from "./globalstate";
 import {fail} from "../utils/utils";
 import {isComputedValue} from "./computedvalue";
@@ -102,19 +103,14 @@ export function isComputingDerivation() {
 	return globalState.trackingDerivation !== null; // filter out actions inside computations
 }
 
-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);
-		}
-	}
+export function checkIfStateModificationsAreAllowed(atom: IAtom) {
+	// TODO: move to messages.ts
+	// Should never be possible to change an observed observable from inside computed, see #798
+	if (globalState.computationDepth > 0 && atom.observers.length > 0)
+		fail(getMessage("m031") + atom.name);
+	// Should not be possible to change observed state outside strict mode, except during initialization, see #563
+	if (!globalState.allowStateChanges && globalState.strictMode && atom.observers.length > 0)
+		fail(getMessage("m030") + atom.name);
 }
 
 /**

src/utils/messages.ts

@@ -25,11 +25,10 @@ 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. 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: " ,
+"m030" : "Since strict-mode is enabled, changing observable values outside actions is not allowed. Please wrap the code in an `action` if this change is intended. 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" ,

test/strict-mode.js

@@ -1,22 +1,45 @@
 var test = require('tape');
 var mobx = require('../');
 
-var strictError = /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/;
+var strictError = /Since strict-mode is enabled, changing observable values outside actions is not allowed. Please wrap the code in an `action` if this change is intended. Tried to modify: /;
 
 test('strict mode should not allow changes outside action', t => {
 	var a = mobx.observable(2);
 	t.equal(mobx.isStrictModeEnabled(), false)
 	mobx.useStrict(true);
 	t.equal(mobx.isStrictModeEnabled(), true)
+
+	// allowed, a is not observed
+	t.doesNotThrow(() => a.set(3), strictError);
+
+	var d = mobx.autorun(() => a.get())
+	// not-allowed, a is observed
 	t.throws(() => a.set(3), strictError);
+	d()
+
 	mobx.useStrict(false);
 	t.equal(mobx.isStrictModeEnabled(), false)
 	a.set(4);
 	t.equal(a.get(), 4);
 	t.end();
 });
 
-test('actions can modify state in strict mode', t => {
+test('actions can modify observed state in strict mode', t => {
+	var a = mobx.observable(2);
+	var d = mobx.autorun(() => a.get())
+
+	mobx.useStrict(true);
+	mobx.action(() => {
+		a.set(3);
+		var b = mobx.observable(4);
+	})();
+
+	mobx.useStrict(false);
+	d()
+	t.end();
+});
+
+test('actions can modify non-observed state in strict mode', t => {
 	var a = mobx.observable(2);
 
 	mobx.useStrict(true);
@@ -35,6 +58,10 @@ test('reactions cannot modify state in strict mode', t => {
 	mobx.useStrict(true);
 	mobx.extras.resetGlobalState(); // should preserve strict mode
 
+	var bd = mobx.autorun(() => {
+		b.get(); // make sure it is observed
+	})
+
 	var d = mobx.autorun(() => {
 		t.throws(() => {
 			a.get();
@@ -52,6 +79,8 @@ test('reactions cannot modify state in strict mode', t => {
 	t.throws(() => a.set(5), strictError);
 
 	mobx.useStrict(false);
+	d()
+	bd()
 	t.end();
 });
 
@@ -60,6 +89,10 @@ test('action inside reaction in strict mode can modify state', t => {
 	var a = mobx.observable(1);
 	var b = mobx.observable(2);
 
+	var bd = mobx.autorun(() => {
+		b.get(); // make sure it is observed
+	})
+
 	mobx.useStrict(true);
 	var act = mobx.action(() => b.set(b.get() + 1));
 
@@ -81,6 +114,8 @@ test('action inside reaction in strict mode can modify state', t => {
 	t.equal(b.get(), 4, "b should not be 55");
 
 	mobx.useStrict(false);
+	bd()
+	d()
 	t.end();
 });
 
@@ -140,6 +175,7 @@ test('can create objects in strict mode with action', t => {
 
 test('strict mode checks', function(t) {
     var x = mobx.observable(3);
+	var d = x.observe(function() {})
 
     mobx.extras.allowStateChanges(false, function() {
         x.get();
@@ -156,5 +192,6 @@ test('strict mode checks', function(t) {
     });
 
 	mobx.extras.resetGlobalState();
+	d()
     t.end();
 });