From db21d85fcd41c6c142998f53e722ee0a0e9b5c18 Mon Sep 17 00:00:00 2001 From: urugator <11457665+urugator@users.noreply.github.com> Date: Thu, 24 Jun 2021 11:55:38 +0200 Subject: [PATCH] trace(): log when computed becomes suspended (#2998) * improve trace * changeset * changeset * adapt mobx-react test --- .changeset/loud-rules-count.md | 6 ++ .../mobx-react/__tests__/issue806.test.tsx | 2 +- packages/mobx/__tests__/v5/base/trace.ts | 12 +++- .../v5/base/typescript-decorators.ts | 60 ++++++++++------- .../__tests__/v5/base/typescript-tests.ts | 66 ++++++++++++------- packages/mobx/src/core/computedvalue.ts | 14 ++-- packages/mobx/src/core/derivation.ts | 6 +- 7 files changed, 109 insertions(+), 57 deletions(-) create mode 100644 .changeset/loud-rules-count.md diff --git a/.changeset/loud-rules-count.md b/.changeset/loud-rules-count.md new file mode 100644 index 000000000..027c5e699 --- /dev/null +++ b/.changeset/loud-rules-count.md @@ -0,0 +1,6 @@ +--- +"mobx": patch +--- + +`trace()`: log when computed becomes suspended +`requiresObserver` warns instead of throwing diff --git a/packages/mobx-react/__tests__/issue806.test.tsx b/packages/mobx-react/__tests__/issue806.test.tsx index 56158d772..244db321b 100644 --- a/packages/mobx-react/__tests__/issue806.test.tsx +++ b/packages/mobx-react/__tests__/issue806.test.tsx @@ -46,7 +46,7 @@ test("verify issue 806", () => { x.a.toString() expect(console.warn).toBeCalledTimes(1) expect(console.warn).toHaveBeenCalledWith( - "[mobx] Observable ObservableObject@1.a being read outside a reactive context" + "[mobx] Observable 'ObservableObject@1.a' being read outside a reactive context." ) }) }) diff --git a/packages/mobx/__tests__/v5/base/trace.ts b/packages/mobx/__tests__/v5/base/trace.ts index a1ade0745..5055fe8a5 100644 --- a/packages/mobx/__tests__/v5/base/trace.ts +++ b/packages/mobx/__tests__/v5/base/trace.ts @@ -35,7 +35,7 @@ describe("trace", () => { x.fullname expectedLogCalls.push([ - "[mobx.trace] 'x.fullname' is being read outside a reactive context. Doing a full recompute" + "[mobx.trace] Computed value 'x.fullname' is being read outside a reactive context. Doing a full recompute." ]) const dispose = mobx.autorun( @@ -61,6 +61,10 @@ describe("trace", () => { dispose() + expectedLogCalls.push([ + "[mobx.trace] Computed value 'x.fullname' was suspended and it will recompute on the next access." + ]) + expect(expectedLogCalls).toEqual(consoleLogSpy.mock.calls) }) @@ -81,7 +85,7 @@ describe("trace", () => { x.fooIsGreaterThan5 expectedLogCalls.push([ - "[mobx.trace] 'x.fooIsGreaterThan5' is being read outside a reactive context. Doing a full recompute" + "[mobx.trace] Computed value 'x.fooIsGreaterThan5' is being read outside a reactive context. Doing a full recompute." ]) const dispose = mobx.autorun( @@ -113,6 +117,10 @@ describe("trace", () => { dispose() + expectedLogCalls.push([ + "[mobx.trace] Computed value 'x.fooIsGreaterThan5' was suspended and it will recompute on the next access." + ]) + expect(expectedLogCalls).toEqual(consoleLogSpy.mock.calls) }) diff --git a/packages/mobx/__tests__/v5/base/typescript-decorators.ts b/packages/mobx/__tests__/v5/base/typescript-decorators.ts index f49362f93..9e6185dc4 100644 --- a/packages/mobx/__tests__/v5/base/typescript-decorators.ts +++ b/packages/mobx/__tests__/v5/base/typescript-decorators.ts @@ -1086,36 +1086,52 @@ test("1072 - @observable without initial value and observe before first access", observe(user, "loginCount", () => {}) }) -test("unread computed reads should trow with requiresReaction enabled", () => { - class A { - @observable x = 0 +test("unobserved computed reads should warn with requiresReaction enabled", () => { + const consoleWarn = console.warn + const warnings: string[] = [] + console.warn = function (...args) { + warnings.push(...args) + } + try { + const expectedWarnings: string[] = [] - @computed({ requiresReaction: true }) - get y() { - return this.x * 2 - } - constructor() { - makeObservable(this) + class A { + @observable x = 0 + + @computed({ requiresReaction: true }) + get y() { + return this.x * 2 + } + constructor() { + makeObservable(this, undefined, { name: "a" }) + } } - } - const a = new A() - expect(() => { + const a = new A() + a.y - }).toThrow(/is read outside a reactive context/) + expectedWarnings.push( + `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.` + ) + + const d = mobx.reaction( + () => a.y, + () => {} + ) - const d = mobx.reaction( - () => a.y, - () => {} - ) - expect(() => { a.y - }).not.toThrow() - d() - expect(() => { + d() + a.y - }).toThrow(/is read outside a reactive context/) + expectedWarnings.push( + `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.` + ) + + expect(warnings).toEqual(expectedWarnings) + } finally { + console.warn = consoleWarn + } }) test("multiple inheritance should work", () => { diff --git a/packages/mobx/__tests__/v5/base/typescript-tests.ts b/packages/mobx/__tests__/v5/base/typescript-tests.ts index ceb25829c..69a1a3cb1 100644 --- a/packages/mobx/__tests__/v5/base/typescript-tests.ts +++ b/packages/mobx/__tests__/v5/base/typescript-tests.ts @@ -1671,39 +1671,57 @@ test("issue #1122", done => { }, 100) }) -test("unread computed reads should trow with requiresReaction enabled", () => { - class A { - x = 0 +test("unobserved computed reads should warn with requiresReaction enabled", () => { + const consoleWarn = console.warn + const warnings: string[] = [] + console.warn = function (...args) { + warnings.push(...args) + } + try { + const expectedWarnings: string[] = [] - constructor() { - makeObservable(this, { - x: observable, - y: computed({ requiresReaction: true }) - }) + class A { + x = 0 + get y() { + return this.x * 2 + } + constructor() { + makeObservable( + this, + { + x: observable, + y: computed({ requiresReaction: true }) + }, + { name: "a" } + ) + } } - get y() { - return this.x * 2 - } - } + const a = new A() - const a = new A() - expect(() => { a.y - }).toThrow(/is read outside a reactive context/) + expectedWarnings.push( + `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.` + ) + + const d = mobx.reaction( + () => a.y, + () => {} + ) - const d = mobx.reaction( - () => a.y, - () => {} - ) - expect(() => { a.y - }).not.toThrow() - d() - expect(() => { + d() + a.y - }).toThrow(/is read outside a reactive context/) + expectedWarnings.push( + `[mobx] Computed value 'a.y' is being read outside a reactive context. Doing a full recompute.` + ) + + expect(warnings).toEqual(expectedWarnings) + } finally { + console.warn = consoleWarn + } }) test("multiple inheritance should work", () => { diff --git a/packages/mobx/src/core/computedvalue.ts b/packages/mobx/src/core/computedvalue.ts index df67e3ddb..86266233d 100644 --- a/packages/mobx/src/core/computedvalue.ts +++ b/packages/mobx/src/core/computedvalue.ts @@ -255,6 +255,11 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva if (!this.keepAlive_) { clearObserving(this) this.value_ = undefined // don't hold on to computed value! + if (this.isTracing_ !== TraceMode.NONE) { + console.log( + `[mobx.trace] Computed value '${this.name_}' was suspended and it will recompute on the next access.` + ) + } } } @@ -283,17 +288,14 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva warnAboutUntrackedRead_() { if (!__DEV__) return - if (this.requiresReaction_ === true) { - die(`[mobx] Computed value ${this.name_} is read outside a reactive context`) - } if (this.isTracing_ !== TraceMode.NONE) { console.log( - `[mobx.trace] '${this.name_}' is being read outside a reactive context. Doing a full recompute` + `[mobx.trace] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.` ) } - if (globalState.computedRequiresReaction) { + if (globalState.computedRequiresReaction || this.requiresReaction_) { console.warn( - `[mobx] Computed value ${this.name_} is being read outside a reactive context. Doing a full recompute` + `[mobx] Computed value '${this.name_}' is being read outside a reactive context. Doing a full recompute.` ) } } diff --git a/packages/mobx/src/core/derivation.ts b/packages/mobx/src/core/derivation.ts index 1b72d25c6..20bb4d1b2 100644 --- a/packages/mobx/src/core/derivation.ts +++ b/packages/mobx/src/core/derivation.ts @@ -149,7 +149,9 @@ export function checkIfStateModificationsAreAllowed(atom: IAtom) { export function checkIfStateReadsAreAllowed(observable: IObservable) { if (__DEV__ && !globalState.allowStateReads && globalState.observableRequiresReaction) { - console.warn(`[mobx] Observable ${observable.name_} being read outside a reactive context`) + console.warn( + `[mobx] Observable '${observable.name_}' being read outside a reactive context.` + ) } } @@ -195,7 +197,7 @@ function warnAboutDerivationWithoutDependencies(derivation: IDerivation) { if (globalState.reactionRequiresObservable || derivation.requiresObservable_) { console.warn( - `[mobx] Derivation ${derivation.name_} is created/updated without reading any observable value` + `[mobx] Derivation '${derivation.name_}' is created/updated without reading any observable value.` ) } }