diff --git a/CHANGELOG.md b/CHANGELOG.md index dda116046..2482c1858 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/api/extendobservable.ts b/src/api/extendobservable.ts index 7d0e2273a..588c84eb1 100644 --- a/src/api/extendobservable.ts +++ b/src/api/extendobservable.ts @@ -23,6 +23,7 @@ export function extendObservable(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)) diff --git a/src/types/modifiers.ts b/src/types/modifiers.ts index 1cc5665b4..3ce091cf0 100644 --- a/src/types/modifiers.ts +++ b/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; } diff --git a/test/makereactive.js b/test/makereactive.js index edccfde00..27396ec2e 100644 --- a/test/makereactive.js +++ b/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/ );