observable(plainObject) now clones, see #649

CHANGELOG.md

@@ -71,6 +71,23 @@ Since this behavior is probably not used outside Mendix, it has been deprecated
 
 See [#621](https://github.com/mobxjs/mobx/issues/621)
 
+### `observable(plainObject)` will no longer enhance objects, but clone instead
+
+When passing a plain object to `observable`, MobX used to modify that object inplace and give it observable capabilities.
+This also happened when assigning a plain object to an observable array etc.
+However, this behavior has changed for a few reasons
+
+1.  Both arrays and maps create new data structure, however, `observable(object)` didn't
+2.  It resulted in unnecessary and confusing side effects. If you passed an object you received from some api to a function that added it, for example, to an observable collection. Suddenly your object would be modified as side effect of passing it down to that function. This was often confusing for beginners and could lead to subtle bugs.
+3.  If MobX in the future uses Proxies behind the scenes, this would need to change as well
+
+If you want, you can still enhance existing plainObjects, but simply using `extendObserable(data, data)`. This was actually the old implementation, which has now changed to `extendObservable({}, data)`.
+
+As always, it is best practice not to have transportation objects etc lingering around; there should be only one source of truth, and that is the data that is in your observable state.
+If you already adhered to this rule, this change won't impact you.
+
+See [#649](https://github.com/mobxjs/mobx/issues/649)
+
 ### Other changes
 
 * Fixed #603: exceptions in transaction breaks future reactions

src/api/extendobservable.ts

@@ -23,6 +23,7 @@ export function extendObservable<A extends Object, B extends Object>(target: A,
 }
 
 export function extendObservableHelper(target, properties, mode: ValueMode, name: string): Object {
+	invariant(Object.isExtensible(target), "Cannot extend the designated object; it is not extensible");
 	const adm = asObservableObject(target, name, mode);
 	for (let key in properties) if (hasOwnProperty(properties, key)) {
 		if (target === properties && !isPropertyConfigurable(target, key))

src/types/modifiers.ts

@@ -121,8 +121,8 @@ export function makeChildObservable(value, parentMode: ValueMode, name?: string)
 
 	if (Array.isArray(value))
 		return createObservableArray(value as any[], childMode, name);
-	if (isPlainObject(value) && Object.isExtensible(value))
-		return extendObservableHelper(value, value, childMode, name);
+	if (isPlainObject(value))
+		return extendObservableHelper({}, value, childMode, name);
 	return value;
 }
 

test/makereactive.js

@@ -32,7 +32,6 @@ test('isObservable', function(t) {
 
     t.equal(m.isObservable(m.observable([])), true);
     t.equal(m.isObservable(m.observable({})), true);
-    t.equal(m.isObservable(m.observable(Object.freeze({}))), false);
     t.equal(m.isObservable(m.observable(function() {})), true);
     t.equal(m.isObservable(m.computed(function() {})), true);
 
@@ -57,8 +56,14 @@ test('isObservable', function(t) {
 
     t.equal(m.isObservable(m.map()), true);
 
-    t.end();
+	const base = { a: 3 };
+	const obs = m.observable(base);
+	t.equal(m.isObservable(base), false);
+	t.equal(m.isObservable(base, "a"), false);
+	t.equal(m.isObservable(obs), true);
+	t.equal(m.isObservable(obs, "a"), true);
 
+    t.end();
 })
 
 test('observable1', function(t) {
@@ -454,7 +459,7 @@ test('ES5 non reactive props', function (t) {
 	 const a = m.extendObservable(te2, { notConfigurable: 1 });
   });
   // should skip non-configurable / writable props when using `observable`
-  te = m.observable(te);
+  te = m.extendObservable(te, te);
   const d1 = Object.getOwnPropertyDescriptor(te, 'nonConfigurable')
   t.equal(d1.value, 'static')
 
@@ -513,6 +518,11 @@ test('exceptions', function(t) {
 })
 
 test("540 - extendobservable should not report cycles", function(t) {
+	t.throws(
+		() => m.extendObservable(Object.freeze({}), {}),
+		/Cannot extend the designated object/
+	);
+
 	var objWrapper = mobx.observable({
 		value: null,
 	});
@@ -523,7 +533,7 @@ test("540 - extendobservable should not report cycles", function(t) {
 
 	objWrapper.value = obj;
 	t.throws(
-		() => mobx.extendObservable(objWrapper, obj),
+		() => mobx.extendObservable(objWrapper, objWrapper.value),
 		/extending an object with another observable \(object\) is not supported/
 	);