Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace(): log when computed becomes suspended #2998

Merged
merged 4 commits into from
Jun 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/loud-rules-count.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"mobx": patch
---

`trace()`: log when computed becomes suspended
`requiresObserver` warns instead of throwing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this change in the PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo ... requiresReaction ... the condition with die is deleted and merged to condition with warn:

 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.`
            )
        }
    }

2 changes: 1 addition & 1 deletion packages/mobx-react/__tests__/issue806.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ test("verify issue 806", () => {
x.a.toString()
expect(console.warn).toBeCalledTimes(1)
expect(console.warn).toHaveBeenCalledWith(
"[mobx] Observable [email protected] being read outside a reactive context"
"[mobx] Observable '[email protected]' being read outside a reactive context."
)
})
})
12 changes: 10 additions & 2 deletions packages/mobx/__tests__/v5/base/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
})

Expand All @@ -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(
Expand Down Expand Up @@ -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)
})

Expand Down
60 changes: 38 additions & 22 deletions packages/mobx/__tests__/v5/base/typescript-decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
66 changes: 42 additions & 24 deletions packages/mobx/__tests__/v5/base/typescript-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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", () => {
Expand Down
14 changes: 8 additions & 6 deletions packages/mobx/src/core/computedvalue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,11 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, 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.`
)
}
}
}

Expand Down Expand Up @@ -283,17 +288,14 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, 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.`
)
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/mobx/src/core/derivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
)
}
}

Expand Down Expand Up @@ -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.`
)
}
}
Expand Down