diff --git a/CHANGELOG.md b/CHANGELOG.md index 160ea8939..d3d4756a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ # 3.0.3 -* Fixed #798: MobX will not throw when invoking an action for computed properties. This is still an anti pattern in general, but caused to often issues when creating new objects in a computed property (often, but not always anti pattern as well) that made use of it's own internal actions +Fixed #798: MobX will not throw when invoking an action during the evaluation of a computed property. +This is still an anti pattern in general, but caused to often issues when creating new objects in a computed property (often, but not always anti pattern as well) that made use of it's own internal actions. +MobX will however still throw if an attempt to change an observable that is already being observed is being changed. # 3.0.2 diff --git a/src/core/computedvalue.ts b/src/core/computedvalue.ts index ccdee7f79..930b76315 100644 --- a/src/core/computedvalue.ts +++ b/src/core/computedvalue.ts @@ -141,6 +141,7 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva computeValue(track: boolean) { this.isComputing = true; + globalState.computationDepth++; const prevAllowStateChanges = allowStateChangesStart(false); let res: T | CaughtException; if (track) { @@ -153,6 +154,7 @@ export class ComputedValue implements IObservable, IComputedValue, IDeriva } } allowStateChangesEnd(prevAllowStateChanges); + globalState.computationDepth--; this.isComputing = false; return res; }; @@ -201,7 +203,7 @@ WhyRun? computation '${this.name}': ` * This computation will re-run if any of the following observables changes: ${joinStrings(observing)} ${(this.isComputing && isTracking) ? " (... or any observable accessed during the remainder of the current run)" : ""} - ${getMessage("m038")} + ${getMessage("m038")} * If the outcome of this computation changes, the following observers will be re-run: ${joinStrings(observers)} diff --git a/src/core/derivation.ts b/src/core/derivation.ts index 0d0a3f57c..461748103 100644 --- a/src/core/derivation.ts +++ b/src/core/derivation.ts @@ -1,6 +1,6 @@ import {IObservable, IDepTreeNode, addObserver, removeObserver} from "./observable"; import {globalState} from "./globalstate"; -import {invariant} from "../utils/utils"; +import {fail} from "../utils/utils"; import {isComputedValue} from "./computedvalue"; import {getMessage} from "../utils/messages"; @@ -102,12 +102,18 @@ export function isComputingDerivation() { return globalState.trackingDerivation !== null; // filter out actions inside computations } -export function checkIfStateModificationsAreAllowed() { - if (!globalState.allowStateChanges) { - invariant(false, globalState.strictMode - ? getMessage("m030") - : getMessage("m031") - ); +export function checkIfStateModificationsAreAllowed(atom: IObservable) { + if (globalState.allowStateChanges === false) { + if (globalState.strictMode) + fail(getMessage("m030") + atom.name); + else + fail(getMessage("m031") + atom.name); + } else { + // Observed observables should not be modified during a computation, + // even not from inside an action! + if (globalState.computationDepth > 0 && atom.observers.length > 0) { + fail(getMessage("m027") + atom.name); + } } } diff --git a/src/core/globalstate.ts b/src/core/globalstate.ts index d27e86780..84f5d12b6 100644 --- a/src/core/globalstate.ts +++ b/src/core/globalstate.ts @@ -21,6 +21,11 @@ export class MobXGlobals { */ trackingDerivation: IDerivation | null = null; + /** + * Are we running a computation currently? (not a reaction) + */ + computationDepth = 0; + /** * Each time a derivation is tracked, it is assigned a unique run-id */ diff --git a/src/types/observablearray.ts b/src/types/observablearray.ts index 992c2d475..71bdbe7e5 100644 --- a/src/types/observablearray.ts +++ b/src/types/observablearray.ts @@ -134,7 +134,7 @@ class ObservableArrayAdministration implements IInterceptable>(adm as any, { diff --git a/src/types/observablevalue.ts b/src/types/observablevalue.ts index 658352c0d..31bdf2314 100644 --- a/src/types/observablevalue.ts +++ b/src/types/observablevalue.ts @@ -63,7 +63,7 @@ export class ObservableValue extends BaseAtom implements IObservableValue, } private prepareNewValue(newValue): T | IUNCHANGED { - checkIfStateModificationsAreAllowed(); + checkIfStateModificationsAreAllowed(this); if (hasInterceptors(this)) { const change = interceptChange>(this, { object: this, type: "update", newValue }); if (!change) diff --git a/src/utils/messages.ts b/src/utils/messages.ts index 6e43638a7..5164cd263 100644 --- a/src/utils/messages.ts +++ b/src/utils/messages.ts @@ -25,10 +25,11 @@ const messages = { "m024" : "whyRun() can only be used if a derivation is active, or by passing an computed value / reaction explicitly. If you invoked whyRun from inside a computation; the computation is currently suspended but re-evaluating because somebody requested its value." , "m025" : "whyRun can only be used on reactions and computed values" , "m026" : "`action` can only be invoked on functions" , +"m027" : "Computed values are not allowed to not cause side effects by changing observables that are already being observed. Tried to change: ", "m028" : "It is not allowed to set `useStrict` when a derivation is running" , "m029" : "INTERNAL ERROR only onBecomeUnobserved shouldn't be called twice in a row" , -"m030" : "It is not allowed to create or change state outside an `action` when MobX is in strict mode. Wrap the current method in `action` if this state change is intended" , -"m031" : "It is not allowed to change the state when a computed value or transformer is being evaluated. Use 'autorun' to create reactive functions with side-effects." , +"m030" : "It is not allowed to create or change state outside an `action` when MobX is in strict mode. Wrap the current method in `action` if this state change is intended. Tried to change: " , +"m031" : "It is not allowed to change the state when a computed value or transformer is being evaluated. Use 'autorun' to create reactive functions with side-effects. Tried to change: " , "m032" : "* This computation is suspended (not in use by any reaction) and won't run automatically.\n Didn't expect this computation to be suspended at this point?\n 1. Make sure this computation is used by a reaction (reaction, autorun, observer).\n 2. Check whether you are using this computation synchronously (in the same stack as they reaction that needs it).", "m033" : "`observe` doesn't support the fire immediately property for observable maps." , "m034" : "`mobx.map` is deprecated, use `new ObservableMap` or `mobx.observable.map` instead" , diff --git a/test/action.js b/test/action.js index 85392f6cd..50dba98a7 100644 --- a/test/action.js +++ b/test/action.js @@ -172,21 +172,49 @@ test('should be possible to create autorun in ation', t => { t.end(); }) -test.skip('should not be possible to invoke action in a computed block', t => { - // Disabled as of #798 +test('should be possible to change unobserved state in an action called from computed', t => { var a = mobx.observable(2); - var noopAction = mobx.action(() => {}); + var testAction = mobx.action(() => { + a.set(3) + }); + + var c = mobx.computed(() => { + testAction(); + }); + + t.plan(1) + mobx.autorun(() => { + t.doesNotThrow(() => { + c.get() + }) + }); + + mobx.extras.resetGlobalState(); + t.end(); +}); + +test('should not be possible to change observed state in an action called from computed', t => { + var a = mobx.observable(2); + var d = mobx.autorun(() => { + a.get() + }) + + var testAction = mobx.action(() => { + a.set(3) + }); var c = mobx.computed(() => { - noopAction(); - return a.get(); + testAction(); + return a.get() }); - utils.consoleError(t, () => { - mobx.autorun(() => c.get()); - }, /Computed values or transformers should not invoke actions or trigger other side effects/, 'expected throw'); + t.throws(() => { + c.get() + }, /Computed values are not allowed to not cause side effects by changing observables that are already being observed/) + mobx.extras.resetGlobalState(); + d(); t.end(); });