Skip to content

Commit

Permalink
safer implementation
Browse files Browse the repository at this point in the history
  • Loading branch information
xaviergonz committed Aug 31, 2018
1 parent 97791b2 commit d619196
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 44 deletions.
20 changes: 4 additions & 16 deletions src/core/action.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,26 +105,14 @@ export function allowStateChangesEnd(prev: boolean) {
globalState.allowStateChanges = prev
}

export function allowStateChangesInsideComputed<T>(
allowStateChangesInsideComputed: boolean,
func: () => T
): T {
const prev = allowStateChangesInsideComputedStart(allowStateChangesInsideComputed)
export function allowStateChangesInsideComputed<T>(func: () => T): T {
const prev = globalState.computationDepth
globalState.computationDepth = 0
let res: T
try {
res = func()
} finally {
allowStateChangesInsideComputedEnd(prev)
globalState.computationDepth += prev
}
return res
}

export function allowStateChangesInsideComputedStart(allowStateChangesInsideComputed: boolean) {
const prev = globalState.allowStateChangesInsideComputed
globalState.allowStateChangesInsideComputed = allowStateChangesInsideComputed
return prev
}

export function allowStateChangesInsideComputedEnd(prev: boolean) {
globalState.allowStateChangesInsideComputed = prev
}
6 changes: 1 addition & 5 deletions src/core/derivation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,7 @@ export function isComputingDerivation() {
export function checkIfStateModificationsAreAllowed(atom: IAtom) {
const hasObservers = atom.observers.size > 0
// Should never be possible to change an observed observable from inside computed, see #798
if (
!globalState.allowStateChangesInsideComputed &&
globalState.computationDepth > 0 &&
hasObservers
)
if (globalState.computationDepth > 0 && hasObservers)
fail(
process.env.NODE_ENV !== "production" &&
`Computed values are not allowed to cause side effects by changing observables that are already being observed. Tried to modify: ${
Expand Down
7 changes: 0 additions & 7 deletions src/core/globalstate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,6 @@ export class MobXGlobals {
*/
allowStateChanges = true

/**
* Is it allowed to change observables when inside a computed context?
* No 99% of the time.
*/
allowStateChangesInsideComputed = false

/**
* If strict mode is enabled, state changes are by default not allowed
*/
Expand Down Expand Up @@ -161,7 +155,6 @@ export function resetGlobalState() {
for (let key in defaultGlobals)
if (persistentKeys.indexOf(key) === -1) globalState[key] = defaultGlobals[key]
globalState.allowStateChanges = !globalState.enforceActions
globalState.allowStateChangesInsideComputed = false
}

declare var window: any
Expand Down
38 changes: 22 additions & 16 deletions test/base/action.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,49 +173,55 @@ test("should be possible to change unobserved state in an action called from com
mobx._resetGlobalState()
})

test("should not be possible to change observed state in an action called from computed", () => {
var a = mobx.observable.box(2)
var d = mobx.autorun(() => {
test("should be possible to change observed state in an action called from computed if run inside _allowStateChangesInsideComputed", () => {
const a = mobx.observable.box(2)
const d = mobx.autorun(() => {
a.get()
})

var testAction = mobx.action(() => {
a.set(3)
const testAction = mobx.action(() => {
mobx._allowStateChangesInsideComputed(() => {
a.set(3)
})
expect(a.get()).toBe(3)
expect(() => {
a.set(4)
}).toThrowError(
/Computed values are not allowed to cause side effects by changing observables that are already being observed/
)
})

var c = mobx.computed(() => {
const c = mobx.computed(() => {
testAction()
return a.get()
})

expect(() => {
c.get()
}).toThrowError(
/Computed values are not allowed to cause side effects by changing observables that are already being observed/
)
c.get()

mobx._resetGlobalState()
d()
})

test("should be possible to change observed state in an action called from computed if run inside _allowStateChangesInsideComputed", () => {
test("should not be possible to change observed state in an action called from computed", () => {
var a = mobx.observable.box(2)
var d = mobx.autorun(() => {
a.get()
})

var testAction = mobx.action(() => {
mobx._allowStateChangesInsideComputed(true, () => {
a.set(3)
})
a.set(3)
})

var c = mobx.computed(() => {
testAction()
return a.get()
})

c.get()
expect(() => {
c.get()
}).toThrowError(
/Computed values are not allowed to cause side effects by changing observables that are already being observed/
)

mobx._resetGlobalState()
d()
Expand Down

0 comments on commit d619196

Please sign in to comment.