Merged #736: improved error handling

CHANGELOG.md

@@ -146,6 +146,16 @@ more consistent behavior:
 
 See [#731](https://github.com/mobxjs/mobx/issues/731)
 
+### Removed error handling, improved error recovery
+
+MobX always printed a warning when an exception was thrown from a computed value, reaction or react component: `[mobx] An uncaught exception occurred while calculating....`.
+This warning was often confusing for people because they either had the impression that this was a mobx exception, while it actually is just informing about an exception that happened in userland code.
+And sometimes, the actual exception was silently caught somewhere else.
+MobX now does not print any warnings anymore, and just makes sure it's internal state is still stable.
+Not throwing or handling an exception is now entirely the responsibility of the user.
+
+Throwing an exception doesn't revert the causing mutation, but it does reset tracking information, which makes it possible to recover from exceptions by changing the state in such a way that a next run of the derivation doesn't throw.
+
 ### Flow-Types Support 🎉🎉🎉
 
 Flow typings have been added by [A-gambit](https://github.com/A-gambit).

src/api/autorun.ts

@@ -1,6 +1,6 @@
 import {Lambda, getNextId, invariant, valueDidChange, fail} from "../utils/utils";
 import {isModifierDescriptor} from "../types/modifiers";
-import {Reaction, IReactionPublic} from "../core/reaction";
+import {Reaction, IReactionPublic, IReactionDisposer} from "../core/reaction";
 import {untrackedStart, untrackedEnd} from "../core/derivation";
 import {action, isAction} from "../api/action";
 
@@ -11,7 +11,7 @@ import {action, isAction} from "../api/action";
  * @param scope (optional)
  * @returns disposer function, which can be used to stop the view from being updated in the future.
  */
-export function autorun(view: (r: IReactionPublic) => void, scope?: any): Lambda;
+export function autorun(view: (r: IReactionPublic) => void, scope?: any): IReactionDisposer;
 
 /**
  * Creates a named reactive view and keeps it alive, so that the view is always
@@ -21,7 +21,7 @@ export function autorun(view: (r: IReactionPublic) => void, scope?: any): Lambda
  * @param scope (optional)
  * @returns disposer function, which can be used to stop the view from being updated in the future.
  */
-export function autorun(name: string, view: (r: IReactionPublic) => void, scope?: any): Lambda;
+export function autorun(name: string, view: (r: IReactionPublic) => void, scope?: any): IReactionDisposer;
 export function autorun(arg1: any, arg2: any, arg3?: any) {
 	let name: string,
 		view: (r: IReactionPublic) => void,
@@ -67,7 +67,7 @@ export function autorun(arg1: any, arg2: any, arg3?: any) {
  * @param scope (optional)
  * @returns disposer function to prematurely end the observer.
  */
-export function when(name: string, predicate: () => boolean, effect: Lambda, scope?: any): Lambda;
+export function when(name: string, predicate: () => boolean, effect: Lambda, scope?: any): IReactionDisposer;
 
 /**
  * Similar to 'observer', observes the given predicate until it returns true.
@@ -104,8 +104,8 @@ export function when(arg1: any, arg2: any, arg3?: any, arg4?: any) {
 	return disposer;
 }
 
-export function autorunAsync(name: string, func: (r: IReactionPublic) => void, delay?: number, scope?: any): Lambda;
-export function autorunAsync(func: (r: IReactionPublic) => void, delay?: number, scope?: any): Lambda;
+export function autorunAsync(name: string, func: (r: IReactionPublic) => void, delay?: number, scope?: any): IReactionDisposer;
+export function autorunAsync(func: (r: IReactionPublic) => void, delay?: number, scope?: any): IReactionDisposer;
 export function autorunAsync(arg1: any, arg2: any, arg3?: any, arg4?: any) {
 	let name: string, func: (r: IReactionPublic) => void, delay: number, scope: any;
 	if (typeof arg1 === "string") {
@@ -163,8 +163,8 @@ export interface IReactionOptions {
  * or
  * autorun(() => action(effect)(expr));
  */
-export function reaction<T>(expression: (r: IReactionPublic) => T, effect: (arg: T, r: IReactionPublic) => void, opts?: IReactionOptions): Lambda;
-export function reaction<T>(expression: (r: IReactionPublic) => T, effect: (arg: T, r: IReactionPublic) => void, fireImmediately?: boolean): Lambda;
+export function reaction<T>(expression: (r: IReactionPublic) => T, effect: (arg: T, r: IReactionPublic) => void, opts?: IReactionOptions): IReactionDisposer;
+export function reaction<T>(expression: (r: IReactionPublic) => T, effect: (arg: T, r: IReactionPublic) => void, fireImmediately?: boolean): IReactionDisposer;
 export function reaction<T>(expression: (r: IReactionPublic) => T, effect: (arg: T, r: IReactionPublic) => void, arg3: any) {
 	if (arguments.length > 3) {
 		fail("reaction only accepts 2 or 3 arguments. If migrating from MobX 2, please provide an options object");

src/api/createtransformer.ts

@@ -24,7 +24,7 @@ export function createTransformer<A, B>(transformer: ITransformer<A, B>, onClean
 			super.onBecomeUnobserved();
 			delete objectCache[this.sourceIdentifier];
 			if (onCleanup)
-				onCleanup(lastValue, this.sourceObject);
+				onCleanup(lastValue as any, this.sourceObject);
 		}
 	}
 

src/core/computedvalue.ts

@@ -1,5 +1,5 @@
 import {IObservable, reportObserved, propagateMaybeChanged, propagateChangeConfirmed, startBatch, endBatch, getObservers} from "./observable";
-import {IDerivation, IDerivationState, trackDerivedFunction, clearObserving, untrackedStart, untrackedEnd, shouldCompute, handleExceptionInDerivation} from "./derivation";
+import {IDerivation, IDerivationState, trackDerivedFunction, clearObserving, untrackedStart, untrackedEnd, shouldCompute, CaughtException, isCaughtException} from "./derivation";
 import {globalState} from "./globalstate";
 import {allowStateChangesStart, allowStateChangesEnd, createAction} from "./action";
 import {createInstanceofPredicate, getNextId, valueDidChange, invariant, Lambda, unique, joinStrings} from "../utils/utils";
@@ -44,7 +44,7 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 	lowestObserverState = IDerivationState.UP_TO_DATE;
 	unboundDepsCount = 0;
 	__mapid = "#" + getNextId();
-	protected value: T | undefined = undefined;
+	protected value: T | undefined | CaughtException = undefined;
 	name: string;
 	isComputing: boolean = false; // to check for cycles
 	isRunningSetter: boolean = false;
@@ -66,28 +66,6 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 			this.setter = createAction(name + "-setter", setter) as any;
 	}
 
-	peek() {
-		this.isComputing = true;
-		const prevAllowStateChanges = allowStateChangesStart(false);
-		const res = this.derivation.call(this.scope);
-		allowStateChangesEnd(prevAllowStateChanges);
-		this.isComputing = false;
-		return res;
-	};
-
-	peekUntracked() {
-		let hasError = true;
-		try {
-			const res = this.peek();
-			hasError = false;
-			return res;
-		} finally {
-			if (hasError)
-				handleExceptionInDerivation(this);
-		}
-
-	}
-
 	onBecomeStale() {
 		propagateMaybeChanged(this);
 	}
@@ -110,25 +88,26 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 			// because it will never be called again inside this batch
 			startBatch();
 			if (shouldCompute(this))
-				this.value = this.peekUntracked();
+				this.value = this.computeValue(false);
 			endBatch();
 		} else {
-
 			reportObserved(this);
 			if (shouldCompute(this))
 				if (this.trackAndCompute())
 					propagateChangeConfirmed(this);
-
 		}
 		const result = this.value!;
 
+		if (isCaughtException(result))
+			throw result.cause;
 		return result;
 	}
 
-	public recoverFromError() {
-		// this.derivation.call(this.scope) in peek returned error, let's run all cleanups, that would be run
-		// note that resetGlobalState will run afterwards
-		this.isComputing = false;
+	public peek(): T {
+		const res = this.computeValue(false);
+		if (isCaughtException(res))
+			throw res.cause;
+		return res;
 	}
 
 	public set(value: T) {
@@ -154,10 +133,28 @@ export class ComputedValue<T> implements IObservable, IComputedValue<T>, IDeriva
 			});
 		}
 		const oldValue = this.value;
-		const newValue = this.value = trackDerivedFunction(this, this.peek);
-		return valueDidChange(this.compareStructural, newValue, oldValue);
+		const newValue = this.value = this.computeValue(true);
+		return isCaughtException(newValue) || valueDidChange(this.compareStructural, newValue, oldValue);
 	}
 
+	computeValue(track: boolean) {
+		this.isComputing = true;
+		const prevAllowStateChanges = allowStateChangesStart(false);
+		let res: T | CaughtException;
+		if (track) {
+			res = trackDerivedFunction(this, this.derivation, this.scope);
+		} else {
+			try {
+				res = this.derivation.call(this.scope);
+			} catch (e) {
+				res = new CaughtException(e);
+			}
+		}
+		allowStateChangesEnd(prevAllowStateChanges);
+		this.isComputing = false;
+		return res;
+	};
+
 	observe(listener: (change: IValueDidChange<T>) => void, fireImmediately?: boolean): Lambda {
 		let firstTime = true;
 		let prevValue: T | undefined = undefined;

src/core/derivation.ts

@@ -1,7 +1,6 @@
-import {IObservable, IDepTreeNode, addObserver, removeObserver, endBatch} from "./observable";
-import {globalState, resetGlobalState} from "./globalstate";
+import {IObservable, IDepTreeNode, addObserver, removeObserver} from "./observable";
+import {globalState} from "./globalstate";
 import {invariant} from "../utils/utils";
-import {isSpyEnabled, spyReport} from "./spy";
 import {isComputedValue} from "./computedvalue";
 
 export enum IDerivationState {
@@ -43,7 +42,16 @@ export interface IDerivation extends IDepTreeNode {
 	unboundDepsCount: number;
 	__mapid: string;
 	onBecomeStale();
-	recoverFromError();
+}
+
+export class CaughtException {
+	constructor(public cause: any) {
+		// Empty
+	}
+}
+
+export function isCaughtException(e): e is CaughtException {
+	return e instanceof CaughtException;
 }
 
 /**
@@ -61,32 +69,29 @@ export function shouldCompute(derivation: IDerivation): boolean {
 		case IDerivationState.UP_TO_DATE: return false;
 		case IDerivationState.NOT_TRACKING: case IDerivationState.STALE: return true;
 		case IDerivationState.POSSIBLY_STALE: {
-			let hasError = true;
 			const prevUntracked = untrackedStart(); // no need for those computeds to be reported, they will be picked up in trackDerivedFunction.
-			try {
-				const obs = derivation.observing, l = obs.length;
-				for (let i = 0; i < l; i++) {
-					const obj = obs[i];
-					if (isComputedValue(obj)) {
+			const obs = derivation.observing, l = obs.length;
+			for (let i = 0; i < l; i++) {
+				const obj = obs[i];
+				if (isComputedValue(obj)) {
+					try {
 						obj.get();
-						// if ComputedValue `obj` actually changed it will be computed and propagated to its observers.
-						// and `derivation` is an observer of `obj`
-						if ((derivation as any).dependenciesState === IDerivationState.STALE) {
-							hasError = false;
-							untrackedEnd(prevUntracked);
-							return true;
-						}
+					} catch (e) {
+						// we are not interested in the value *or* exception at this moment, but if there is one, notify all
+						untrackedEnd(prevUntracked);
+						return true;
+					}
+					// if ComputedValue `obj` actually changed it will be computed and propagated to its observers.
+					// and `derivation` is an observer of `obj`
+					if ((derivation as any).dependenciesState === IDerivationState.STALE) {
+						untrackedEnd(prevUntracked);
+						return true;
 					}
-				}
-				hasError = false;
-				changeDependenciesStateTo0(derivation);
-				untrackedEnd(prevUntracked);
-				return false;
-			} finally {
-				if (hasError) {
-					changeDependenciesStateTo0(derivation);
 				}
 			}
+			changeDependenciesStateTo0(derivation);
+			untrackedEnd(prevUntracked);
+			return false;
 		}
 	}
 }
@@ -109,7 +114,7 @@ export function checkIfStateModificationsAreAllowed() {
  * The tracking information is stored on the `derivation` object and the derivation is registered
  * as observer of any of the accessed observables.
  */
-export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T) {
+export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T, context) {
 	// pre allocate array allocation + room for variation in deps
 	// array will be trimmed by bindDependencies
 	changeDependenciesStateTo0(derivation);
@@ -118,47 +123,15 @@ export function trackDerivedFunction<T>(derivation: IDerivation, f: () => T) {
 	derivation.runId = ++globalState.runId;
 	const prevTracking = globalState.trackingDerivation;
 	globalState.trackingDerivation = derivation;
-	let hasException = true;
-	let result: T;
+	let result;
 	try {
-		result = f.call(derivation);
-		hasException = false;
-		return result;
-	} finally {
-		if (hasException) {
-			handleExceptionInDerivation(derivation);
-		} else {
-			globalState.trackingDerivation = prevTracking;
-			bindDependencies(derivation);
-		}
-	}
-}
-
-export function handleExceptionInDerivation(derivation: IDerivation) {
-	const message = (
-		`[mobx] An uncaught exception occurred while calculating your computed value, autorun or transformer. Or inside the render() method of an observer based React component. ` +
-		`These functions should never throw exceptions as MobX will not always be able to recover from them. ` +
-		`Please fix the error reported after this message or enable 'Pause on (caught) exceptions' in your debugger to find the root cause. In: '${derivation.name}'. ` +
-		`For more details see https://github.com/mobxjs/mobx/issues/462`
-	);
-	if (isSpyEnabled()) {
-		spyReport({
-			type: "error",
-			message
-		});
+		result = f.call(context);
+	} catch (e) {
+		result = new CaughtException(e);
 	}
-	console.warn(message); // In next major, maybe don't emit this message at all?
-	// Poor mans recovery attempt
-	// Assumption here is that this is the only exception handler in MobX.
-	// So functions higher up in the stack (like transanction) won't be modifying the globalState anymore after this call.
-	// (Except for other trackDerivedFunction calls of course, but that is just)
-	changeDependenciesStateTo0(derivation);
-	derivation.newObserving = null;
-	derivation.unboundDepsCount = 0;
-	derivation.recoverFromError();
-	// close current batch, make sure pending unobservations are executed
-	endBatch();
-	resetGlobalState();
+	globalState.trackingDerivation = prevTracking;
+	bindDependencies(derivation);
+	return result;
 }
 
 /**

src/core/globalstate.ts

@@ -1,5 +1,5 @@
 import {getGlobal} from "../utils/utils";
-import {IDerivation} from "./derivation";
+import {IDerivation, CaughtException} from "./derivation";
 import {Reaction} from "./reaction";
 import {IObservable} from "./observable";
 
@@ -49,6 +49,11 @@ export class MobXGlobals {
 	 */
 	pendingReactions: Reaction[] = [];
 
+	/**
+	 * Are we currently processing reactions?
+	 */
+	isRunningReactions = false;
+
 	/**
 	 * Is it allowed to change observables at this point?
 	 * In general, MobX doesn't allow that when running computations and React.render.
@@ -69,6 +74,11 @@ export class MobXGlobals {
 	 * Spy callbacks
 	 */
 	spyListeners: {(change: any): void}[] = [];
+
+	/**
+	 * Globally attached error handlers that react specifically to errors in reactions
+	 */
+	globalReactionErrorHandlers: ((error: any, derivation: IDerivation) => void)[] = [];
 }
 
 export let globalState: MobXGlobals = new MobXGlobals();