From 366798eaf84b13707f64271c7130e73df79f518e Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:03:53 +0100 Subject: [PATCH 1/8] Removes constraint against using extendObservable on an existing property --- src/api/extendobservable.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/api/extendobservable.ts b/src/api/extendobservable.ts index 301782b63..a1b29f9d7 100644 --- a/src/api/extendobservable.ts +++ b/src/api/extendobservable.ts @@ -86,12 +86,6 @@ export function extendObservableObjectWithProperties( if (process.env.NODE_ENV !== "production") { if (!isPlainObject(properties)) fail(`'extendObservabe' only accepts plain objects as second argument`) - if (Object.getOwnPropertyDescriptor(target, key)) - fail( - `'extendObservable' can only be used to introduce new properties. Use 'set' or 'decorate' instead. The property '${stringifyKey( - key - )}' already exists on '${target}'` - ) if (isComputed(descriptor.value)) fail( `Passing a 'computed' as initial property value is no longer supported by extendObservable. Use a getter or decorator instead` From cbb2dd45767489c10ba38b91b0ebf292386a704b Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:04:55 +0100 Subject: [PATCH 2/8] Adds unit tests for extendObservable --- test/base/extendObservable.js | 119 ++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 test/base/extendObservable.js diff --git a/test/base/extendObservable.js b/test/base/extendObservable.js new file mode 100644 index 000000000..9e3396c95 --- /dev/null +++ b/test/base/extendObservable.js @@ -0,0 +1,119 @@ +// @ts-check + +import { + computed, + autorun, + isObservable, + isObservableProp, + isComputedProp, + extendObservable +} from "../../src/mobx" + +test("extendObservable should work", function() { + class Box { + // @ts-ignore + uninitialized + height = 20 + sizes = [2] + someFunc = function() { + return 2 + } + get width() { + return ( + this.undeclared * + this.height * + this.sizes.length * + this.someFunc() * + (this.uninitialized ? 2 : 1) + ) + } + addSize() { + // @ts-ignore + this.sizes.push([3]) + // @ts-ignore + this.sizes.push([4]) + } + constructor() { + this.undeclared = 1 + } + } + + const box = new Box() + + extendObservable(box, { + height: 20, + sizes: [2], + get someFunc() { + return 2 + }, + width: 40 + }) + + expect(isObservableProp(box, "height")).toBe(true) + expect(isObservableProp(box, "sizes")).toBe(true) + expect(isObservable(box.sizes)).toBe(true) + expect(isObservableProp(box, "someFunc")).toBe(true) + expect(isComputedProp(box, "someFunc")).toBe(true) + expect(isObservableProp(box, "width")).toBe(true) + + const ar = [] + + autorun(() => { + ar.push(box.width) + }) + + expect(ar.slice()).toEqual([40]) +}) + +test("extendObservable should work with plain object", function() { + const box = { + /** @type {boolean | undefined} */ + uninitialized: undefined, + height: 20, + sizes: [2], + someFunc: function() { + return 2 + }, + get width() { + return ( + this.undeclared * + this.height * + this.sizes.length * + this.someFunc() * + (this.uninitialized ? 2 : 1) + ) + }, + addSize() { + // @ts-ignore + this.sizes.push([3]) + // @ts-ignore + this.sizes.push([4]) + } + } + + box.undeclared = 1 + + extendObservable(box, { + height: 20, + sizes: [2], + get someFunc() { + return 2 + }, + width: 40 + }) + + expect(isObservableProp(box, "height")).toBe(true) + expect(isObservableProp(box, "sizes")).toBe(true) + expect(isObservable(box.sizes)).toBe(true) + expect(isObservableProp(box, "someFunc")).toBe(true) + expect(isComputedProp(box, "someFunc")).toBe(true) + expect(isObservableProp(box, "width")).toBe(true) + + const ar = [] + + autorun(() => { + ar.push(box.width) + }) + + expect(ar.slice()).toEqual([40]) +}) From 0330fb80057539928d3c1836852565bb75280b0e Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:05:36 +0100 Subject: [PATCH 3/8] Updates/removes unit tests which assumed extendObservable constraint --- test/base/makereactive.js | 11 ++++---- test/base/observables.js | 53 +++++++++++++++++++---------------- test/base/proxies.js | 39 +++++++++++++++----------- test/base/typescript-tests.ts | 46 +++++++++++++++--------------- 4 files changed, 80 insertions(+), 69 deletions(-) diff --git a/test/base/makereactive.js b/test/base/makereactive.js index a301a5c08..eaa8e0e39 100644 --- a/test/base/makereactive.js +++ b/test/base/makereactive.js @@ -228,7 +228,11 @@ test("observable5", function() { return 3 } x.nonReactive = three - expect(b.toArray()).toEqual([[17, f, 17], [18, f, 18], [18, three, 3]]) + expect(b.toArray()).toEqual([ + [17, f, 17], + [18, f, 18], + [18, three, 3] + ]) }) test("flat array", function() { @@ -650,11 +654,6 @@ test("double declare property", () => { mobx.extendObservable(o, { a: 5 }) - expect(() => { - mobx.extendObservable(o, { - a: 3 - }) - }).toThrow(/can only be used to introduce new properties/) expect(() => { mobx.extendObservable( o, diff --git a/test/base/observables.js b/test/base/observables.js index 376865f66..073a016c6 100644 --- a/test/base/observables.js +++ b/test/base/observables.js @@ -1344,7 +1344,11 @@ test("atom events #427", () => { let stop = 0 let runs = 0 - const a = mobx.createAtom("test", () => start++, () => stop++) + const a = mobx.createAtom( + "test", + () => start++, + () => stop++ + ) expect(a.reportObserved()).toEqual(false) expect(start).toBe(0) @@ -1682,12 +1686,18 @@ test("computed equals function only invoked when necessary", () => { // Another value change will cause a comparison right.set("F") - expect(comparisons).toEqual([{ from: "ab", to: "cb" }, { from: "de", to: "df" }]) + expect(comparisons).toEqual([ + { from: "ab", to: "cb" }, + { from: "de", to: "df" } + ]) // Becoming unobserved, then observed won't cause a comparison disposeAutorun() disposeAutorun = mobx.autorun(() => values.push(combinedToLowerCase.get())) - expect(comparisons).toEqual([{ from: "ab", to: "cb" }, { from: "de", to: "df" }]) + expect(comparisons).toEqual([ + { from: "ab", to: "cb" }, + { from: "de", to: "df" } + ]) expect(values).toEqual(["ab", "cb", "de", "df", "df"]) @@ -1754,23 +1764,6 @@ test("Issue 1120 - isComputed should return false for a non existing property", expect(mobx.isComputedProp(observable({}), "x")).toBe(false) }) -test("It should not be possible to redefine a computed property", () => { - const a = observable({ - width: 10, - get surface() { - return this.width - } - }) - - expect(() => { - mobx.extendObservable(a, { - get surface() { - return this.width * 2 - } - }) - }).toThrow(/'extendObservable' can only be used to introduce new properties/) -}) - test("extendObservable should not be able to set a computed property", () => { expect(() => { observable({ @@ -1817,7 +1810,10 @@ test("computed comparer works with decorate (plain)", () => { time.minute = 0 expect(changes).toEqual([{ hour: 9, minute: 0 }]) time.hour = 10 - expect(changes).toEqual([{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + expect(changes).toEqual([ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 expect(changes).toEqual([ { hour: 9, minute: 0 }, @@ -1856,7 +1852,10 @@ test("computed comparer works with decorate (plain) - 2", () => { time.minute = 0 expect(changes).toEqual([{ hour: 9, minute: 0 }]) time.hour = 10 - expect(changes).toEqual([{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + expect(changes).toEqual([ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 expect(changes).toEqual([ { hour: 9, minute: 0 }, @@ -1891,7 +1890,10 @@ test("computed comparer works with decorate (plain) - 3", () => { time.minute = 0 expect(changes).toEqual([{ hour: 9, minute: 0 }]) time.hour = 10 - expect(changes).toEqual([{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + expect(changes).toEqual([ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 expect(changes).toEqual([ { hour: 9, minute: 0 }, @@ -2073,7 +2075,10 @@ test("tuples", () => { const myStuff = tuple(1, 3) const events = [] - mobx.reaction(() => myStuff[0], val => events.push(val)) + mobx.reaction( + () => myStuff[0], + val => events.push(val) + ) myStuff[1] = 17 // should not react myStuff[0] = 2 // should react expect(events).toEqual([2]) diff --git a/test/base/proxies.js b/test/base/proxies.js index afbd5cd67..3b317039d 100644 --- a/test/base/proxies.js +++ b/test/base/proxies.js @@ -20,7 +20,11 @@ test("should react to key removal (unless reconfiguraing to empty) - 1", () => { z: 1 }) - reaction(() => Object.keys(x), keys => events.push(keys.join(",")), { fireImmediately: true }) + reaction( + () => Object.keys(x), + keys => events.push(keys.join(",")), + { fireImmediately: true } + ) expect(events).toEqual(["y,z"]) delete x.y expect(events).toEqual(["y,z", "z"]) @@ -36,7 +40,10 @@ test("should react to key removal (unless reconfiguraing to empty) - 2", () => { z: 1 }) - reaction(() => x.z, v => events.push(v)) + reaction( + () => x.z, + v => events.push(v) + ) delete x.z expect(events).toEqual([undefined]) @@ -49,7 +56,10 @@ test("should react to key removal (unless reconfiguraing to empty) - 2", () => { z: undefined }) - reaction(() => x.z, v => events.push(v)) + reaction( + () => x.z, + v => events.push(v) + ) delete x.z expect(events).toEqual([]) @@ -59,7 +69,10 @@ test("should react to future key additions - 1", () => { const events = [] const x = observable.object({}) - reaction(() => Object.keys(x), keys => events.push(keys.join(","))) + reaction( + () => Object.keys(x), + keys => events.push(keys.join(",")) + ) x.y = undefined expect(events).toEqual(["y"]) @@ -88,18 +101,6 @@ test("should react to future key additions - 2", () => { expect(events).toEqual([4]) }) -test("should throw clear warning if trying to add computed to already reserved key", () => { - const x = observable.object({ z: 3 }) - - expect(() => { - extendObservable(x, { - get z() { - return 3 - } - }) - }).toThrow(/can only be used to introduce new properties/) -}) - test("correct keys are reported", () => { const x = observable.object({ x: 1, @@ -119,7 +120,11 @@ test("correct keys are reported", () => { expect(Object.keys(x)).toEqual(["x", "z", "a"]) expect(Object.values(x)).toEqual([1, 3, 4]) - expect(Object.entries(x)).toEqual([["x", 1], ["z", 3], ["a", 4]]) + expect(Object.entries(x)).toEqual([ + ["x", 1], + ["z", 3], + ["a", 4] + ]) expect(Object.getOwnPropertyNames(x)).toEqual(["x", "y", "z", "a", "b"]) expect(keys(x)).toEqual(["x", "z", "a"]) diff --git a/test/base/typescript-tests.ts b/test/base/typescript-tests.ts index 7f0304c00..73e15e9db 100644 --- a/test/base/typescript-tests.ts +++ b/test/base/typescript-tests.ts @@ -212,22 +212,6 @@ const state: any = observable({ authToken: null }) -test("issue8", () => { - t.throws(() => { - class LoginStoreTest { - loggedIn2: boolean = false - constructor() { - extendObservable(this, { - get loggedIn2() { - return true - } - }) - } - } - const store = new LoginStoreTest() - }, /'extendObservable' can only be used to introduce new properties/) -}) - test("box", () => { class Box { @observable uninitialized: any @@ -1206,7 +1190,10 @@ test("@computed.equals (TS)", () => { time.minute = 0 t.deepEqual(changes, [{ hour: 9, minute: 0 }]) time.hour = 10 - t.deepEqual(changes, [{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + t.deepEqual(changes, [ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 t.deepEqual(changes, [ { hour: 9, minute: 0 }, @@ -1242,7 +1229,10 @@ test("computed comparer works with decorate (TS)", () => { time.minute = 0 t.deepEqual(changes, [{ hour: 9, minute: 0 }]) time.hour = 10 - t.deepEqual(changes, [{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + t.deepEqual(changes, [ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 t.deepEqual(changes, [ { hour: 9, minute: 0 }, @@ -1278,7 +1268,10 @@ test("computed comparer works with decorate (TS)", () => { time.minute = 0 t.deepEqual(changes, [{ hour: 9, minute: 0 }]) time.hour = 10 - t.deepEqual(changes, [{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + t.deepEqual(changes, [ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 t.deepEqual(changes, [ { hour: 9, minute: 0 }, @@ -1323,7 +1316,10 @@ test("computed comparer works with decorate (TS) - 2", () => { time.minute = 0 t.deepEqual(changes, [{ hour: 9, minute: 0 }]) time.hour = 10 - t.deepEqual(changes, [{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + t.deepEqual(changes, [ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 t.deepEqual(changes, [ { hour: 9, minute: 0 }, @@ -1358,7 +1354,10 @@ test("computed comparer works with decorate (TS) - 3", () => { time.minute = 0 t.deepEqual(changes, [{ hour: 9, minute: 0 }]) time.hour = 10 - t.deepEqual(changes, [{ hour: 9, minute: 0 }, { hour: 10, minute: 0 }]) + t.deepEqual(changes, [ + { hour: 9, minute: 0 }, + { hour: 10, minute: 0 } + ]) time.minute = 30 t.deepEqual(changes, [ { hour: 9, minute: 0 }, @@ -1465,7 +1464,10 @@ test("unread computed reads should trow with requiresReaction enabled", () => { a.y }).toThrow(/is read outside a reactive context/) - const d = mobx.reaction(() => a.y, () => {}) + const d = mobx.reaction( + () => a.y, + () => {} + ) expect(() => { a.y }).not.toThrow() From 0e275da4d9707a981c9c53b1593a8abef3643cd2 Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:13:59 +0100 Subject: [PATCH 4/8] Updates changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d3bff41d..9c8d07397 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 5.15.3 / 4.15.3 [NOT PUBLISHED YET] + +- Fixes #2250 - extendObservable: allows it to be used on an existing property [#2250](https://github.com/mobxjs/mobx/pull/2250) + # 5.15.2 / 4.15.2 [NOT PUBLISHED YET] - Fixes #2230 - computedvalue: throw error object instead of string when options is empty [#2243](https://github.com/mobxjs/mobx/pull/2243) From 20d8a11b69bb1ac509f6e20addbdf2aaa11ef600 Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:18:28 +0100 Subject: [PATCH 5/8] Updates docs --- docs/refguide/api.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/refguide/api.md b/docs/refguide/api.md index 307dbd52a..5f016368a 100644 --- a/docs/refguide/api.md +++ b/docs/refguide/api.md @@ -116,7 +116,7 @@ The api is further the same as the [ES6 set api](https://developer.mozilla.org/e Usage: `extendObservable(target, properties, decorators?, options?)`. -For each key/value pair in each `properties` a (new) observable property will be introduced on the target object. +For each key/value pair in each `properties` a (new) observable property will be introduced on the target object if the property does not already exist on the target object. If the property does exist, it will be made observable. This can be used in constructor functions to introduce observable properties without using decorators. If a value of the `properties` is a getter function, a _computed_ property will be introduced. From 783630efb6db6d4d20a6b9b67c63cfc5c76edbef Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:31:19 +0100 Subject: [PATCH 6/8] Updates changelog with correct pull request id --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c8d07397..fb963c005 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # 5.15.3 / 4.15.3 [NOT PUBLISHED YET] -- Fixes #2250 - extendObservable: allows it to be used on an existing property [#2250](https://github.com/mobxjs/mobx/pull/2250) +- Fixes #2250 - extendObservable: allows it to be used on an existing property [#2252](https://github.com/mobxjs/mobx/pull/2252) # 5.15.2 / 4.15.2 [NOT PUBLISHED YET] From 3e89c3e099d23951b779c911d5385bfa592c8067 Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 11:35:13 +0100 Subject: [PATCH 7/8] Updates changelog to put in previous unreleased version --- CHANGELOG.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb963c005..7fa336a01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,12 +1,10 @@ -# 5.15.3 / 4.15.3 [NOT PUBLISHED YET] - -- Fixes #2250 - extendObservable: allows it to be used on an existing property [#2252](https://github.com/mobxjs/mobx/pull/2252) - # 5.15.2 / 4.15.2 [NOT PUBLISHED YET] - Fixes #2230 - computedvalue: throw error object instead of string when options is empty [#2243](https://github.com/mobxjs/mobx/pull/2243) - supports ES6 Sets and Maps in shallow comparer. [#2238](https://github.com/mobxjs/mobx/pull/2238) +- Fixes #2250 - extendObservable: allows it to be used on an existing property [#2252](https://github.com/mobxjs/mobx/pull/2252) + # 5.15.1 / 4.15.1 - Make initial values of observable set accept readonly array [#2202](https://github.com/mobxjs/mobx/pull/2202) From df1fcf99529114baa0901ec43faeb70caa36ea74 Mon Sep 17 00:00:00 2001 From: Dave Feucht Date: Thu, 9 Jan 2020 13:28:57 +0100 Subject: [PATCH 8/8] Adds test for providing type of decorator to extendObservable --- test/base/extendObservable.js | 45 ++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/test/base/extendObservable.js b/test/base/extendObservable.js index 9e3396c95..68ac40bfc 100644 --- a/test/base/extendObservable.js +++ b/test/base/extendObservable.js @@ -1,11 +1,12 @@ // @ts-check import { - computed, + action, autorun, isObservable, isObservableProp, isComputedProp, + isAction, extendObservable } from "../../src/mobx" @@ -117,3 +118,45 @@ test("extendObservable should work with plain object", function() { expect(ar.slice()).toEqual([40]) }) + +test("extendObservable should apply specified decorators", function() { + const box = { + /** @type {boolean | undefined} */ + uninitialized: undefined, + height: 20, + sizes: [2], + someFunc: function() { + return 2 + }, + get width() { + return ( + this.undeclared * + this.height * + this.sizes.length * + this.someFunc() * + (this.uninitialized ? 2 : 1) + ) + }, + addSize() { + // @ts-ignore + this.sizes.push([3]) + // @ts-ignore + this.sizes.push([4]) + } + } + + box.undeclared = 1 + + extendObservable( + box, + { + someFunc: function() { + return 2 + } + }, + { someFunc: action } + ) + + expect(isAction(box.someFunc)).toBe(true) + expect(box.someFunc()).toEqual(2) +})