feat: Flexible Error/Exception handling (#5929)

* feat: trace.error() implementation

* refactor: Based on PR review

* chore: adding error-handling guide

* docs: fix typos

CONTRIBUTING.md

@@ -61,11 +61,12 @@ git checkout -b <my-fix-branch> master
 
 4. The fun part! Make your code changes. Make sure you:
     - Follow the [code conventions guide](CodingConvention.md).
+    - Follow the [guide on handling errors and exceptions](HandlingErrors.md).
     - Write unit tests for your fix or feature. Check out [writing unit tests guide](WritingUnitTests.md).
 
 5. Before you submit your PR:
     - Rebase your changes to the latest master: `git pull --rebase upstream master`.
-    - Ensure all unit test are green for Android and iOS. Check [running unit tests](DevelopmentWorkflow.md#running-unit-tests).
+    - Ensure all unit test are green for Android and iOS. Check [running unit   tests](DevelopmentWorkflow.md#running-unit-tests).
     - Ensure your changes pass tslint validation. (run `npm run tslint` in the root of the repo).
 
 6. Push your fork. If you have rebased you might have to use force-push your branch:

HandlingErrors.md

@@ -0,0 +1,36 @@
+# Handling Errors in NativeScript Core Modules
+
+One big difference between web-app and NativeScript app is that throwing an `Error` in NativeScript app causes the app to **crash**. Such crashes can be a serious problem for a production applications as they hurt the application credibility and drive away customers.
+
+We want to provide the application developers with the flexibility to handle errors differently in *development* and *production* modes.
+
+## Using the Trace Module
+The `tns-core-modules/trace` utility module provides a good way to streamline error logging and handling throughout the framework. It gives application developers a way to define custom `TraceWriter`s and `ErrorHandler`s for their apps and even specify different sets of those to be used in during development and in production.
+
+Here are the guidelines how to use this when contributing to core-modules or creating your own plugins.
+
+### Use `trace.write()`
+Use trace.write() with the appropriate type to log non critical errors. 
+
+>Note: For the `error` message level all loggers will be notified unconditionally, for all other levels (`log`,`info`,`warn`), tracing should be enabled and the corresponding categories should be added.
+
+### Use `trace.error()`
+Using `trace.error()` gives the user of you API for a [flexible way of handling the errors](https://github.com/NativeScript/NativeScript/issues/5914).
+
+Use the `error()` when an error has occurred which compromises the stability of the app. The default `ErrorHandler` provided in the `trace` module will throw the error which will include the stack trace and information useful for debugging during development. Application developers can handle this error using a custom `ErrorHandler` in production and decide if they should trigger a crash, send an error report, try to recover the application in other way or combination of those. 
+
+After calling `trace.error()` consider just returning from the function you are currently in without completing. 
+
+There are cases when code execution jumps between native code (ex. Android/iOS SDKs) and JavaScript trough callbacks. In those cases it is most difficult to determine if an error (ex. expected argument is `undefined` or current state of components is invalid) is critical or not. Although, it seems that error is unrecoverable, it might be the case that the callback is called when the app has gone to the background or trough activity/window that is not longer visible. So just reporting the error with `write()` or `error()` is a good option in such cases.
+
+
+## Throw the Error directly in code
+Avoid throwing errors directly, especially in code that is not directly called from application developers (for example in properties set in markup/CSS or in callbacks called form native code). This will cause a crash and usually it will be hard for users of the code to `try/catch` and handle the error. Resort to throwing for cases when: 
+
+1. Continuing execution will cause data loss or corruption. Compromising future runs of the application or persisting corrupt data is even worse than crashing.
+2. Obviously misused public APIs (ex. wrong arguments types) which developers will call directly.  
+
+## Clearing Legacy Code
+Not all the code in the `tns-core-modules` might conform to this guide as it might be written before some of the improvements of the trace modules (ex. `error()`). If you came across to such code you can always [give us a PR](CONTRIBUTING.md) referencing [this issue](https://github.com/NativeScript/NativeScript/issues/5914).
+
+

tests/app/testRunner.ts

@@ -99,6 +99,9 @@ allTests["FILE-NAME-RESOLVER"] = fileNameResolverTests;
 import * as weakEventsTests from "./ui/core/weak-event-listener/weak-event-listener-tests";
 allTests["WEAK-EVENTS"] = weakEventsTests;
 
+import * as traceErrorTests from "./trace/trace-error-tests";
+allTests["TRACE-ERROR"] = traceErrorTests;
+
 import * as connectivityTests from "./connectivity/connectivity-tests";
 allTests["CONNECTIVITY"] = connectivityTests;
 

tests/app/trace/trace-error-tests.ts

@@ -0,0 +1,67 @@
+import {
+    ErrorHandler, getErrorHandler, setErrorHandler, DefaultErrorHandler,
+    error as traceError
+} from "tns-core-modules/trace";
+import * as TKUnit from "../TKUnit";
+
+let cachedErrorHandler: ErrorHandler;
+export function setUpModule() {
+    cachedErrorHandler = getErrorHandler();
+}
+
+// before each
+export function tearDown() {
+    setErrorHandler(cachedErrorHandler)
+}
+
+export function test_DefaultErrorHandler_throws() {
+    setErrorHandler(new DefaultErrorHandler());
+    TKUnit.assertThrows(() => {
+        traceError(new Error("TEST"))
+    }, "DefaultErrorHandler should throw.", "TEST")
+}
+
+export function test_trace_error_should_call_handler() {
+    let called = false;
+    setErrorHandler({
+        handlerError() {
+            called = true;
+        }
+    });
+    traceError(new Error("TEST"));
+
+    TKUnit.assert(called, "trace.error() should call handler")
+}
+
+export function test_trace_error_should_create_error_from_string() {
+    let called = false;
+    let actualError: Error;
+    setErrorHandler({
+        handlerError(error) {
+            called = true;
+            actualError = error;
+        }
+    });
+    traceError("TEST");
+
+    TKUnit.assert(called, "trace.error() should call handler;")
+    TKUnit.assert(actualError instanceof Error, "trace.error() wrap string in error")
+}
+
+export function test_trace_error_should_pass_errors() {
+    let called = false;
+    let testError = new Error("TEST");
+    let actualError: Error;
+
+    setErrorHandler({
+        handlerError(error) {
+            called = true;
+            actualError = error;
+
+        }
+    });
+    traceError(testError);
+
+    TKUnit.assert(called, "trace.error() should call handler;")
+    TKUnit.assertDeepEqual(actualError, testError)
+}

tns-core-modules/trace/trace.d.ts

@@ -63,6 +63,11 @@ export function isCategorySet(category: string): boolean;
  */
 export function write(message: any, category: string, type?: number);
 
+/**
+ * Passes an error to the registered ErrorHandler
+ * @param error The error to be handled.
+ */
+export function error(error: string | Error);
 /**
  * Notifies all the attached listeners for an event that has occurred in the sender object.
  * @param object The Object instance that raised the event.
@@ -75,6 +80,10 @@ export function addEventListener(listener: EventListener);
 
 export function removeEventListener(listener: EventListener);
 
+export function getErrorHandler(): ErrorHandler;
+
+export function setErrorHandler(handler: ErrorHandler);
+
 /**
  * An enum that defines all predefined categories.
  */
@@ -122,3 +131,14 @@ export interface EventListener {
     filter: string;
     on(object: Object, name: string, data?: any);
 }
+
+/**
+ * An interface used to for handling trace error  
+ */
+export interface ErrorHandler {
+    handlerError(error: Error);
+}
+
+export class DefaultErrorHandler implements ErrorHandler {
+    handlerError(error);
+}

tns-core-modules/trace/trace.ts

@@ -1,9 +1,10 @@
-import * as definition from ".";
+import { EventListener, TraceWriter, ErrorHandler } from ".";
 
 let enabled = false;
 let _categories = {};
-let _writers: Array<definition.TraceWriter> = [];
-let _eventListeners: Array<definition.EventListener> = [];
+let _writers: Array<TraceWriter> = [];
+let _eventListeners: Array<EventListener> = [];
+let _errorHandler: ErrorHandler;
 
 export function enable() {
     enabled = true;
@@ -21,11 +22,11 @@ export function isCategorySet(category: string): boolean {
     return category in _categories;
 }
 
-export function addWriter(writer: definition.TraceWriter) {
+export function addWriter(writer: TraceWriter) {
     _writers.push(writer);
 }
 
-export function removeWriter(writer: definition.TraceWriter) {
+export function removeWriter(writer: TraceWriter) {
     let index = _writers.indexOf(writer);
     if (index >= 0) {
         _writers.splice(index, 1);
@@ -79,7 +80,7 @@ export function notifyEvent(object: Object, name: string, data?: any) {
     }
 
     let i,
-        listener: definition.EventListener,
+        listener: EventListener,
         filters: Array<string>;
     for (i = 0; i < _eventListeners.length; i++) {
         listener = _eventListeners[i];
@@ -96,11 +97,11 @@ export function notifyEvent(object: Object, name: string, data?: any) {
     }
 }
 
-export function addEventListener(listener: definition.EventListener) {
+export function addEventListener(listener: EventListener) {
     _eventListeners.push(listener);
 }
 
-export function removeEventListener(listener: definition.EventListener) {
+export function removeEventListener(listener: EventListener) {
     var index = _eventListeners.indexOf(listener);
     if (index >= 0) {
         _eventListeners.splice(index, 1);
@@ -148,7 +149,7 @@ export module categories {
     }
 }
 
-class ConsoleWriter implements definition.TraceWriter {
+class ConsoleWriter implements TraceWriter {
     public write(message: any, category: string, type?: number) {
         if (!console) {
             return;
@@ -177,6 +178,31 @@ class ConsoleWriter implements definition.TraceWriter {
         }
     }
 }
-
 // register a ConsoleWriter by default
 addWriter(new ConsoleWriter());
+
+export class DefaultErrorHandler implements ErrorHandler {
+    handlerError(error) {
+        throw error;
+    }
+}
+setErrorHandler(new DefaultErrorHandler());
+
+export function getErrorHandler(): ErrorHandler {
+    return _errorHandler;
+}
+
+export function setErrorHandler(handler: ErrorHandler) {
+    _errorHandler = handler;
+}
+export function error(error: string | Error) {
+    if (!_errorHandler) {
+        return;
+    }
+
+    if (typeof error === "string") {
+        error = new Error(error);
+    }
+
+    _errorHandler.handlerError(error);
+}