From 2380320263f5edcd06d7ba6bdf02aff3fd7d305a Mon Sep 17 00:00:00 2001 From: urugator <11457665+urugator@users.noreply.github.com> Date: Fri, 21 Jan 2022 19:44:58 +0100 Subject: [PATCH] Fix #3271 flow.bound (#3273) --- .changeset/fuzzy-coins-beam.md | 5 + .../mobx/__tests__/v5/base/make-observable.ts | 107 ++++++++++++------ packages/mobx/src/types/flowannotation.ts | 14 ++- 3 files changed, 86 insertions(+), 40 deletions(-) create mode 100644 .changeset/fuzzy-coins-beam.md diff --git a/.changeset/fuzzy-coins-beam.md b/.changeset/fuzzy-coins-beam.md new file mode 100644 index 000000000..eaf7faa9f --- /dev/null +++ b/.changeset/fuzzy-coins-beam.md @@ -0,0 +1,5 @@ +--- +"mobx": patch +--- + +fix `flow.bound` #3271 diff --git a/packages/mobx/__tests__/v5/base/make-observable.ts b/packages/mobx/__tests__/v5/base/make-observable.ts index 182e7320a..3e3091d83 100644 --- a/packages/mobx/__tests__/v5/base/make-observable.ts +++ b/packages/mobx/__tests__/v5/base/make-observable.ts @@ -378,7 +378,7 @@ test("makeAutoObservable allows overrides", () => { }) test("makeAutoObservable cannot be used on subclasses", () => { - class A { } + class A {} class B extends A { constructor() { @@ -1232,10 +1232,10 @@ test("subclass - cannot re-annotate", () => { computed: computed }) } - action() { } - actionBound() { } - *flow() { } - *flowBound() { } + action() {} + actionBound() {} + *flow() {} + *flowBound() {} get computed() { return this } @@ -1248,7 +1248,7 @@ test("subclass - cannot re-annotate", () => { action: action }) } - action() { } + action() {} } class ChildActionBound extends Parent { @@ -1258,7 +1258,7 @@ test("subclass - cannot re-annotate", () => { actionBound: action.bound }) } - actionBound() { } + actionBound() {} } class ChildFlow extends Parent { @@ -1268,7 +1268,7 @@ test("subclass - cannot re-annotate", () => { flow: flow }) } - *flow() { } + *flow() {} } class ChildFlowBound extends Parent { @@ -1278,7 +1278,7 @@ test("subclass - cannot re-annotate", () => { flowBound: flow.bound }) } - *flowBound() { } + *flowBound() {} } class ChildObservable extends Parent { @@ -1319,13 +1319,13 @@ test("subclass - cannot re-decorate", () => { makeObservable(this) } @action - action() { } + action() {} @action.bound - actionBound() { } + actionBound() {} @flow - *flow() { } + *flow() {} @flow.bound - *flowBound() { } + *flowBound() {} @computed get computed() { return this @@ -1339,7 +1339,7 @@ test("subclass - cannot re-decorate", () => { makeObservable(this) } @action - action() { } + action() {} } }).toThrow(/^\[MobX\] Cannot apply/) @@ -1350,7 +1350,7 @@ test("subclass - cannot re-decorate", () => { makeObservable(this) } @action.bound - actionBound() { } + actionBound() {} } }).toThrow(/^\[MobX\] Cannot apply/) @@ -1361,7 +1361,7 @@ test("subclass - cannot re-decorate", () => { makeObservable(this) } @flow - *flow() { } + *flow() {} } }).toThrow(/^\[MobX\] Cannot apply/) @@ -1372,7 +1372,7 @@ test("subclass - cannot re-decorate", () => { makeObservable(this) } @flow.bound - *flowBound() { } + *flowBound() {} } }).toThrow(/^\[MobX\] Cannot apply/) @@ -1411,14 +1411,14 @@ test("subclass - cannot redefine property", () => { computed: computed }) } - action = () => { } + action = () => {} get computed() { return this } } class ChildAction extends Parent { - action = () => { } + action = () => {} } class ChildObservable extends Parent { @@ -1560,16 +1560,16 @@ test("makeAutoObservable + production build #2751", () => { test("inherited fields are assignable before makeObservable", () => { class Foo { constructor() { - this.action = () => { } - this.flow = function* flow() { } + this.action = () => {} + this.flow = function* flow() {} makeObservable(this, { action, flow }) } - action() { } - *flow() { } + action() {} + *flow() {} } const foo1 = new Foo() @@ -1592,7 +1592,7 @@ test("makeAutoObservable + symbolic keys", () => { get [computedSymbol]() { return this.observable } - [actionSymbol]() { } + [actionSymbol]() {} constructor() { makeAutoObservable(this) @@ -1652,20 +1652,53 @@ test("makeObservable throws when mixing @decorators with annotations", () => { } } - expect(() => new Test()).toThrow(/makeObservable second arg must be nullish when using decorators/); + expect(() => new Test()).toThrow( + /makeObservable second arg must be nullish when using decorators/ + ) }) test("makeAutoObservable + Object.create #3197", () => { const proto = { - action() { }, - *flow() { }, - get computed() { return null }, - }; - const o = Object.create(proto); - o.observable = 5; - makeAutoObservable(o); - expect(isAction(proto.action)).toBe(true); - expect(isFlow(proto.flow)).toBe(true); - expect(isComputedProp(o, "computed")).toBe(true); - expect(isObservableProp(o, "observable")).toBe(true); -}); \ No newline at end of file + action() {}, + *flow() {}, + get computed() { + return null + } + } + const o = Object.create(proto) + o.observable = 5 + makeAutoObservable(o) + expect(isAction(proto.action)).toBe(true) + expect(isFlow(proto.flow)).toBe(true) + expect(isComputedProp(o, "computed")).toBe(true) + expect(isObservableProp(o, "observable")).toBe(true) +}) + +test("flow.bound #3271", async () => { + class Test { + constructor() { + makeObservable(this, { flowBound: flow.bound }) + } + *flowBound() { + return this + } + } + + const t1 = new Test() + const t2 = new Test() + + // Make sure flow is actually bindable + expect( + await flow(function* () { + return this + }).bind(t1)() + ).toBe(t1) + + expect(t1.hasOwnProperty("flowBound")).toBe(true) + expect(t2.hasOwnProperty("flowBound")).toBe(true) + + expect(t1.flowBound !== t2.flowBound).toBe(true) + + expect(await t1.flowBound.call(null)).toBe(t1) + expect(await t2.flowBound.call(null)).toBe(t2) +}) diff --git a/packages/mobx/src/types/flowannotation.ts b/packages/mobx/src/types/flowannotation.ts index 5ddd76131..65f80b29d 100644 --- a/packages/mobx/src/types/flowannotation.ts +++ b/packages/mobx/src/types/flowannotation.ts @@ -7,7 +7,8 @@ import { isFlow, isFunction, globalState, - MakeResult + MakeResult, + hasProp } from "../internal" export function createFlowAnnotation(name: string, options?: object): Annotation { @@ -33,7 +34,7 @@ function make_( } // prototype // bound - must annotate protos to support super.flow() - if (this.options_?.bound && !isFlow(adm.target_[key])) { + if (this.options_?.bound && (!hasProp(adm.target_, key) || !isFlow(adm.target_[key]))) { if (this.extend_(adm, key, descriptor, false) === null) return MakeResult.Cancel } if (isFlow(descriptor.value)) { @@ -81,11 +82,18 @@ function createFlowDescriptor( ): PropertyDescriptor { assertFlowDescriptor(adm, annotation, key, descriptor) let { value } = descriptor + // In case of flow.bound, the descriptor can be from already annotated prototype + if (!isFlow(value)) { + value = flow(value) + } if (bound) { + // We do not keep original function around, so we bind the existing flow value = value.bind(adm.proxy_ ?? adm.target_) + // This is normally set by `flow`, but `bind` returns new function... + value.isMobXFlow = true } return { - value: flow(value), + value, // Non-configurable for classes // prevents accidental field redefinition in subclass configurable: safeDescriptors ? adm.isPlainObject_ : true,