Merge pull request #588 from mobxjs/fix-540-extend-observable-cycle

Fixed #540 by explicitly forbidding extending with observable objects

CHANGELOG.md

@@ -1,3 +1,7 @@
+# 2.6.0
+
+* Extending observable objects with other observable (objects) is now explicitly forbidden, fixes #540.
+
 # 2.5.2
 
 * Introduced `isComputed`

src/api/extendobservable.ts

@@ -1,6 +1,7 @@
 import {ValueMode} from "../types/modifiers";
 import {isObservableMap} from "../types/observablemap";
 import {asObservableObject, setObservableObjectInstanceProperty} from "../types/observableobject";
+import {isObservable} from "../api/isobservable";
 import {invariant, isPropertyConfigurable, hasOwnProperty} from "../utils/utils";
 
 /**
@@ -15,6 +16,7 @@ export function extendObservable<A extends Object, B extends Object>(target: A,
 	invariant(!(isObservableMap(target)), "extendObservable should not be used on maps, use map.merge instead");
 	properties.forEach(propSet => {
 		invariant(typeof propSet === "object", "all arguments of extendObservable should be objects");
+		invariant(!isObservable(propSet), "extending an object with another observable (object) is not supported. Please construct an explicit propertymap, using `toJS` if need. See issue #540");
 		extendObservableHelper(target, propSet, ValueMode.Recursive, null);
 	});
 	return <A & B> target;

test/makereactive.js

@@ -473,10 +473,10 @@ test('ES5 non reactive props', function (t) {
   });
   const d2 = Object.getOwnPropertyDescriptor(te2, 'notWritable')
   t.equal(d2.value, 'static')
-  
+
   // should not throw for other props
   t.equal(m.extendObservable(te, { 'bla' : 3}).bla, 3);
-  
+
   t.end();
 })
 
@@ -513,3 +513,24 @@ test('exceptions', function(t) {
 
     return t.end();
 })
+
+test("540 - extendobservable should not report cycles", function(t) {
+	var objWrapper = mobx.observable({
+		value: null,
+	});
+
+	var obj = {
+		name: "Hello",
+	};
+
+	objWrapper.value = obj;
+	t.throws(
+		() => mobx.extendObservable(objWrapper, obj),
+		/extending an object with another observable \(object\) is not supported/
+	);
+
+	mobx.autorun(() => {
+		console.log(objWrapper.name);
+	});
+	t.end();
+})