From d5e7589533ded01c714bea2ae571e488c6cac10c Mon Sep 17 00:00:00 2001 From: James Zhan Date: Wed, 10 Jul 2024 15:19:15 +1000 Subject: [PATCH 1/7] fix action on field --- .../decorators_20223/stage3-decorators.ts | 32 ++++++++++++------- packages/mobx/src/types/actionannotation.ts | 28 ++++++++-------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts b/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts index e76c8f9fb..2caba5249 100644 --- a/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts +++ b/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts @@ -334,9 +334,7 @@ function normalizeSpyEvents(events: any[]) { test("action decorator (2022.3)", () => { class Store { - constructor(private multiplier: number) { - makeObservable(this) - } + constructor(private multiplier: number) {} @action add(a: number, b: number): number { @@ -366,9 +364,7 @@ test("action decorator (2022.3)", () => { test("custom action decorator (2022.3)", () => { class Store { - constructor(private multiplier: number) { - makeObservable(this) - } + constructor(private multiplier: number) {} @action("zoem zoem") add(a: number, b: number): number { @@ -416,9 +412,7 @@ test("custom action decorator (2022.3)", () => { test("action decorator on field (2022.3)", () => { class Store { - constructor(private multiplier: number) { - makeObservable(this) - } + constructor(private multiplier: number) {} @action add = (a: number, b: number) => { @@ -450,9 +444,7 @@ test("action decorator on field (2022.3)", () => { test("custom action decorator on field (2022.3)", () => { class Store { - constructor(private multiplier: number) { - makeObservable(this) - } + constructor(private multiplier: number) {} @action("zoem zoem") add = (a: number, b: number) => { @@ -893,6 +885,22 @@ test("action.bound binds (2022.3)", () => { t.equal(a.x, 2) }) +test("action.bound binds property function (2022.3)", () => { + class A { + @observable accessor x = 0 + @action.bound + inc = function (value: number) { + this.x += value + } + } + + const a = new A() + const runner = a.inc + runner(2) + + t.equal(a.x, 2) +}) + test("@computed.equals (2022.3)", () => { const sameTime = (from: Time, to: Time) => from.hour === to.hour && from.minute === to.minute class Time { diff --git a/packages/mobx/src/types/actionannotation.ts b/packages/mobx/src/types/actionannotation.ts index 831cf5236..32ec42785 100644 --- a/packages/mobx/src/types/actionannotation.ts +++ b/packages/mobx/src/types/actionannotation.ts @@ -73,28 +73,28 @@ function decorate_20223_(this: Annotation, mthd, context: DecoratorContext) { const _createAction = m => createAction(ann.options_?.name ?? name!.toString(), m, ann.options_?.autoAction ?? false) - // Backwards/Legacy behavior, expects makeObservable(this) - if (kind == "field") { + if ((kind == "field" || kind == "method") && this.options_?.bound) { addInitializer(function () { - storeAnnotation(this, name, ann) + const self = this as any + const bound = self[name].bind(self) + bound.isMobxAction = true + self[name] = bound }) - return + } + + if (kind == "field") { + return initMthd => { + if (!isAction(initMthd)) { + return _createAction(initMthd) + } + return initMthd + } } if (kind == "method") { if (!isAction(mthd)) { mthd = _createAction(mthd) } - - if (this.options_?.bound) { - addInitializer(function () { - const self = this as any - const bound = self[name].bind(self) - bound.isMobxAction = true - self[name] = bound - }) - } - return mthd } From 11e554a3c48f2eb04822a52598040fd86886575e Mon Sep 17 00:00:00 2001 From: James Zhan Date: Wed, 10 Jul 2024 15:21:58 +1000 Subject: [PATCH 2/7] Create soft-beans-dream.md --- .changeset/soft-beans-dream.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/soft-beans-dream.md diff --git a/.changeset/soft-beans-dream.md b/.changeset/soft-beans-dream.md new file mode 100644 index 000000000..c11821bc5 --- /dev/null +++ b/.changeset/soft-beans-dream.md @@ -0,0 +1,5 @@ +--- +"mobx": patch +--- + +Fix 2022.3 @action decorators on fields no longer require makeObservable From 4303d8b35d18e5800da1001d3a178905969867ef Mon Sep 17 00:00:00 2001 From: James Zhan Date: Wed, 10 Jul 2024 15:28:08 +1000 Subject: [PATCH 3/7] cleanup --- packages/mobx/src/types/actionannotation.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/mobx/src/types/actionannotation.ts b/packages/mobx/src/types/actionannotation.ts index 32ec42785..ec023783c 100644 --- a/packages/mobx/src/types/actionannotation.ts +++ b/packages/mobx/src/types/actionannotation.ts @@ -8,8 +8,7 @@ import { Annotation, globalState, MakeResult, - assert20223DecoratorType, - storeAnnotation + assert20223DecoratorType } from "../internal" export function createActionAnnotation(name: string, options?: object): Annotation { From b02bc215e08f9ce4edaa82a22d9d54cfe3ce8f2d Mon Sep 17 00:00:00 2001 From: James Zhan Date: Wed, 10 Jul 2024 17:39:41 +1000 Subject: [PATCH 4/7] update doc --- docs/enabling-decorators.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/enabling-decorators.md b/docs/enabling-decorators.md index e864f07ad..44b51751e 100644 --- a/docs/enabling-decorators.md +++ b/docs/enabling-decorators.md @@ -14,8 +14,9 @@ After years of alterations, ES decorators have finally reached Stage 3 in the TC With modern decorators, it is no longer needed to call `makeObservable` / `makeAutoObservable`. 2022.3 Decorators are supported in: -* TypeScript (5.0 and higher, make sure that the `experimentalDecorators` flag is NOT enabled). [Example commit](https://github.com/mweststrate/currencies-demo/commit/acb9ac8c148e8beef88042c847bb395131e85d60). -* For Babel make sure the plugin [`proposal-decorators`](https://babeljs.io/docs/babel-plugin-proposal-decorators) is enabled with the highest version (currently `2023-05`). [Example commit](https://github.com/mweststrate/currencies-demo/commit/4999d2228208f3e1e10bc00a272046eaefde8585). + +- TypeScript (5.0 and higher, make sure that the `experimentalDecorators` flag is NOT enabled). [Example commit](https://github.com/mweststrate/currencies-demo/commit/acb9ac8c148e8beef88042c847bb395131e85d60). +- For Babel make sure the plugin [`proposal-decorators`](https://babeljs.io/docs/babel-plugin-proposal-decorators) is enabled with the highest version (currently `2023-05`). [Example commit](https://github.com/mweststrate/currencies-demo/commit/4999d2228208f3e1e10bc00a272046eaefde8585). ```js // tsconfig.json @@ -106,6 +107,7 @@ class TodoList { } } ``` +
Migrating from legacy decorators @@ -126,7 +128,6 @@ MobX' 2022.3 Decorators are very similar to the MobX 5 decorators, so usage is m The main cases for enumerability seem to have been around serialization and rest destructuring. - Regarding serialization, implicitly serializing all properties probably isn't ideal in an OOP-world anyway, so this doesn't seem like a substantial issue (consider implementing `toJSON` or using `serializr` as possible alternatives) - Addressing rest-destructuring, such is an anti-pattern in MobX - doing so would (likely unwantedly) touch all observables and make the observer overly-reactive). -- `@action some_field = () => {}` was and is valid usage (_if_ `makeObservable()` is also used). However, `@action accessor some_field = () => {}` is never valid.
From 483d562a55188a471203c379261bba7b0c19dab2 Mon Sep 17 00:00:00 2001 From: James Zhan Date: Thu, 11 Jul 2024 14:58:13 +1000 Subject: [PATCH 5/7] tidy up --- packages/mobx/src/types/actionannotation.ts | 32 ++++++++++++--------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/packages/mobx/src/types/actionannotation.ts b/packages/mobx/src/types/actionannotation.ts index ec023783c..f8e6177e8 100644 --- a/packages/mobx/src/types/actionannotation.ts +++ b/packages/mobx/src/types/actionannotation.ts @@ -72,21 +72,17 @@ function decorate_20223_(this: Annotation, mthd, context: DecoratorContext) { const _createAction = m => createAction(ann.options_?.name ?? name!.toString(), m, ann.options_?.autoAction ?? false) - if ((kind == "field" || kind == "method") && this.options_?.bound) { - addInitializer(function () { - const self = this as any - const bound = self[name].bind(self) - bound.isMobxAction = true - self[name] = bound - }) - } - if (kind == "field") { - return initMthd => { - if (!isAction(initMthd)) { - return _createAction(initMthd) + return function (initMthd) { + let mthd = initMthd + if (!isAction(mthd)) { + mthd = _createAction(initMthd) + } + if (ann.options_?.bound) { + mthd = mthd.bind(this) + mthd.isMobxAction = true } - return initMthd + return mthd } } @@ -94,6 +90,16 @@ function decorate_20223_(this: Annotation, mthd, context: DecoratorContext) { if (!isAction(mthd)) { mthd = _createAction(mthd) } + + if (this.options_?.bound) { + addInitializer(function () { + const self = this as any + const bound = self[name].bind(self) + bound.isMobxAction = true + self[name] = bound + }) + } + return mthd } From c49e87ca8f9c46e51fb3372bbb1603007fc8967b Mon Sep 17 00:00:00 2001 From: James Zhan Date: Fri, 12 Jul 2024 11:16:12 +1000 Subject: [PATCH 6/7] minor fix --- packages/mobx/src/types/actionannotation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/mobx/src/types/actionannotation.ts b/packages/mobx/src/types/actionannotation.ts index f8e6177e8..ab9d354bb 100644 --- a/packages/mobx/src/types/actionannotation.ts +++ b/packages/mobx/src/types/actionannotation.ts @@ -76,7 +76,7 @@ function decorate_20223_(this: Annotation, mthd, context: DecoratorContext) { return function (initMthd) { let mthd = initMthd if (!isAction(mthd)) { - mthd = _createAction(initMthd) + mthd = _createAction(mthd) } if (ann.options_?.bound) { mthd = mthd.bind(this) From b6b84df0943db1bc6ba9a3345dfacd5075b3d411 Mon Sep 17 00:00:00 2001 From: James Zhan Date: Wed, 11 Sep 2024 10:09:52 +1000 Subject: [PATCH 7/7] add doc and unit test --- docs/enabling-decorators.md | 3 +++ .../decorators_20223/stage3-decorators.ts | 22 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/docs/enabling-decorators.md b/docs/enabling-decorators.md index 44b51751e..aaa2ad976 100644 --- a/docs/enabling-decorators.md +++ b/docs/enabling-decorators.md @@ -128,6 +128,9 @@ MobX' 2022.3 Decorators are very similar to the MobX 5 decorators, so usage is m The main cases for enumerability seem to have been around serialization and rest destructuring. - Regarding serialization, implicitly serializing all properties probably isn't ideal in an OOP-world anyway, so this doesn't seem like a substantial issue (consider implementing `toJSON` or using `serializr` as possible alternatives) - Addressing rest-destructuring, such is an anti-pattern in MobX - doing so would (likely unwantedly) touch all observables and make the observer overly-reactive). +- `@action some_field = () => {}` was and is valid usage. However, inheritance is different between legacy decorators and modern decorators. + - In legacy decorators, if superclass has a field decorated by `@action`, and subclass tries to override the same field, it will throw a `TypeError: Cannot redefine property`. + - In modern decorators, if superclass has a field decorated by `@action`, and subclass tries to override the same field, it's allowed to override the field. However, the field on subclass is not an action unless it's also decorated with `@action` in subclass declaration. diff --git a/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts b/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts index 2caba5249..9f2420d6e 100644 --- a/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts +++ b/packages/mobx/__tests__/decorators_20223/stage3-decorators.ts @@ -1093,3 +1093,25 @@ test("#2159 - computed property keys", () => { "original string value" // original string ]) }) + +test(`decorated field can be inherited, but doesn't inherite the effect of decorator`, () => { + class Store { + @action + action = () => { + return + } + } + + class SubStore extends Store { + action = () => { + // should not be a MobX action + return + } + } + + const store = new Store() + expect(isAction(store.action)).toBe(true) + + const subStore = new SubStore() + expect(isAction(subStore.action)).toBe(false) +})