From e67a17bef036a770813fadb257d8e3670fb4ba22 Mon Sep 17 00:00:00 2001
From: Thierry Michel <thmichel@gmail.com>
Date: Wed, 6 Nov 2019 18:47:32 +0100
Subject: [PATCH] feat(core): :loud_sound: add/improve error logs

Closes #447
---
 .../core/__tests__/core/core.once.test.ts     | 21 ---------
 .../core/__tests__/core/core.page.test.ts     | 14 +++---
 packages/core/__tests__/hooks/hooks.test.ts   | 17 +++++++
 packages/core/__tests__/modules/error.test.ts | 20 ++++++++
 .../transitions/transitions.context.test.ts   |  2 +-
 .../transitions/transitions.once.test.ts      | 22 +++++----
 .../transitions/transitions.page.test.ts      | 47 +++++++++++++++----
 packages/core/src/core.ts                     | 22 +++++----
 packages/core/src/hooks.ts                    |  5 +-
 packages/core/src/modules/Error.ts            | 20 ++++++++
 packages/core/src/modules/Transitions.ts      | 32 +++++++++----
 packages/core/src/utils/run-async.ts          |  1 +
 12 files changed, 158 insertions(+), 65 deletions(-)
 create mode 100644 packages/core/__tests__/modules/error.test.ts
 create mode 100644 packages/core/src/modules/Error.ts

diff --git a/packages/core/__tests__/core/core.once.test.ts b/packages/core/__tests__/core/core.once.test.ts
index 650267d3..a1ab5fe2 100644
--- a/packages/core/__tests__/core/core.once.test.ts
+++ b/packages/core/__tests__/core/core.once.test.ts
@@ -26,24 +26,3 @@ it('do once', async () => {
   expect(spyOnce).toHaveBeenCalledTimes(1);
   spyOnce.mockRestore();
 });
-
-it('catches error', async () => {
-  expect.assertions(2);
-  const errorOnce = new Error('Once error');
-  const errorTransition = new Error('Transition error [once]');
-  const t = {
-    once() {
-      throw errorOnce;
-    },
-  };
-  barba.transitions.store.add('transition', t);
-  Logger.setLevel('error');
-
-  barba.logger.error = jest.fn();
-  barba.transitions.logger.error = jest.fn();
-
-  await barba.once(data);
-
-  expect(barba.transitions.logger.error).toHaveBeenCalledWith(errorOnce);
-  expect(barba.logger.error).toHaveBeenCalledWith(errorTransition);
-});
diff --git a/packages/core/__tests__/core/core.page.test.ts b/packages/core/__tests__/core/core.page.test.ts
index 2ec14223..8793d701 100644
--- a/packages/core/__tests__/core/core.page.test.ts
+++ b/packages/core/__tests__/core/core.page.test.ts
@@ -3,8 +3,12 @@ import xhrMock from 'xhr-mock';
 import { init } from '../../__mocks__/barba';
 import barba from '../../src';
 import { hooks } from '../../src/hooks';
+import { Logger } from '../../src/modules/Logger';
 import { schemaAttribute } from '../../src/schemas/attribute';
 
+// Silence is gold… :)
+Logger.setLevel('off');
+
 // Needed for "request" module
 (global as any).Headers = class {};
 
@@ -141,14 +145,11 @@ it('do page [waiting]', async () => {
   expect(barba.data.next.html).toMatch(checkDoc);
 });
 
-it('catches error', async () => {
+it('force on error', async () => {
   expect.assertions(2);
-  barba.logger.error = jest.fn();
+  barba.force = jest.fn();
   barba.transitions.logger.error = jest.fn();
-  barba.history.cancel = jest.fn();
-  spyPage.mockRestore();
   const errorLeave = new Error('Transition error [page][leave]');
-  const errorTransition = new Error('Transition error');
 
   barba.transitions.store.add('transition', {
     leave() {
@@ -159,6 +160,5 @@ it('catches error', async () => {
   await barba.page(nextUrl, 'barba', false);
 
   expect(barba.transitions.logger.error).toHaveBeenCalledWith(errorLeave);
-  expect(barba.logger.error).toHaveBeenCalledWith(errorTransition);
-  // expect(spyCacheGetAction).toHaveBeenCalledTimes(1);
+  expect(barba.force).toHaveBeenCalledTimes(1);
 });
diff --git a/packages/core/__tests__/hooks/hooks.test.ts b/packages/core/__tests__/hooks/hooks.test.ts
index 6d965681..e68a88a2 100644
--- a/packages/core/__tests__/hooks/hooks.test.ts
+++ b/packages/core/__tests__/hooks/hooks.test.ts
@@ -99,3 +99,20 @@ it('print help', () => {
 
   expect(hooks.logger.info).toHaveBeenCalledTimes(2);
 });
+
+it('catch error', async () => {
+  expect.assertions(2);
+  hooks.logger.debug = jest.fn();
+  hooks.logger.error = jest.fn();
+
+  const err = new Error('Test error');
+
+  hooks.init();
+  hooks[hookName](() => {
+    throw err;
+  });
+  await hooks.do(hookName);
+
+  expect(hooks.logger.debug).toHaveBeenCalledWith(`Hook error [${hookName}]`);
+  expect(hooks.logger.error).toHaveBeenCalledWith(err);
+});
diff --git a/packages/core/__tests__/modules/error.test.ts b/packages/core/__tests__/modules/error.test.ts
new file mode 100644
index 00000000..2a3cf17f
--- /dev/null
+++ b/packages/core/__tests__/modules/error.test.ts
@@ -0,0 +1,20 @@
+import { BarbaError } from '../../src/modules/Error';
+
+const err = new Error('Test error');
+
+it('has defaults', () => {
+  const e = new BarbaError(err);
+
+  expect(e.name).toBe('BarbaError');
+  expect(e.label).toBe('Barba error');
+  expect(e.error).toBe(err);
+});
+
+it('has params', () => {
+  const e = new BarbaError(err, 'Label error', 'Message error');
+
+  expect(e.name).toBe('BarbaError');
+  expect(e.label).toBe('Label error');
+  expect(e.message).toBe('Message error');
+  expect(e.error).toBe(err);
+});
diff --git a/packages/core/__tests__/modules/transitions/transitions.context.test.ts b/packages/core/__tests__/modules/transitions/transitions.context.test.ts
index 7eb25d84..fd37dcd6 100644
--- a/packages/core/__tests__/modules/transitions/transitions.context.test.ts
+++ b/packages/core/__tests__/modules/transitions/transitions.context.test.ts
@@ -1,7 +1,7 @@
 import {
   ISchemaPage,
-  ITransitionOnce,
   ITransitionData,
+  ITransitionOnce,
 } from '../../../src/defs';
 import { Logger } from '../../../src/modules/Logger';
 import { Transitions } from '../../../src/modules/Transitions';
diff --git a/packages/core/__tests__/modules/transitions/transitions.once.test.ts b/packages/core/__tests__/modules/transitions/transitions.once.test.ts
index e4e38c2e..00966401 100644
--- a/packages/core/__tests__/modules/transitions/transitions.once.test.ts
+++ b/packages/core/__tests__/modules/transitions/transitions.once.test.ts
@@ -78,20 +78,22 @@ it('calls hooks', async () => {
 
 it('catches error', async () => {
   expect.assertions(2);
+  transitions.logger.debug = jest.fn();
   transitions.logger.error = jest.fn();
 
+  const err = new Error('Test');
   const onceError = () => {
-    throw new Error('Test');
+    throw err;
   };
   const t = { once: onceError };
 
-  try {
-    await transitions.doOnce({
-      data,
-      transition: t,
-    });
-  } catch (e) {
-    expect(e).toEqual(new Error('Transition error [once]'));
-    expect(transitions.isRunning).toBeFalsy();
-  }
+  await transitions.doOnce({
+    data,
+    transition: t,
+  });
+
+  expect(transitions.logger.debug).toHaveBeenCalledWith(
+    'Transition error [before/after/once]'
+  );
+  expect(transitions.logger.error).toHaveBeenCalledWith(err);
 });
diff --git a/packages/core/__tests__/modules/transitions/transitions.page.test.ts b/packages/core/__tests__/modules/transitions/transitions.page.test.ts
index 0cd97b00..dfdc8bcd 100644
--- a/packages/core/__tests__/modules/transitions/transitions.page.test.ts
+++ b/packages/core/__tests__/modules/transitions/transitions.page.test.ts
@@ -1,5 +1,6 @@
 /* tslint:disable:no-empty */
 import { init } from '../../../__mocks__/barba';
+import barba from '../../../src';
 import { ISchemaPage, ITransitionData } from '../../../src/defs';
 import { hooks } from '../../../src/hooks';
 import { Logger } from '../../../src/modules/Logger';
@@ -197,7 +198,7 @@ it('calls hooks (sync: true)', async () => {
 });
 
 it('catches error (leave, sync: false)', async () => {
-  expect.assertions(2);
+  expect.assertions(4);
 
   const leaveError = () => {
     throw new Error('test');
@@ -212,13 +213,15 @@ it('catches error (leave, sync: false)', async () => {
       wrapper,
     });
   } catch (e) {
-    expect(e).toEqual(new Error('Transition error'));
+    expect(e.name).toEqual('BarbaError');
+    expect(e.label).toEqual('Transition error [before/after/leave]');
+    expect(e.error).toEqual(new Error('test'));
     expect(transitions.isRunning).toBeFalsy();
   }
 });
 
 it('catches error (enter, sync: false)', async () => {
-  expect.assertions(2);
+  expect.assertions(4);
 
   const enterError = () => {
     throw new Error('test');
@@ -238,13 +241,15 @@ it('catches error (enter, sync: false)', async () => {
       wrapper,
     });
   } catch (e) {
-    expect(e).toEqual(new Error('Transition error'));
+    expect(e.name).toEqual('BarbaError');
+    expect(e.label).toEqual('Transition error [before/after/enter]');
+    expect(e.error).toEqual(new Error('test'));
     expect(transitions.isRunning).toBeFalsy();
   }
 });
 
 it('catches error (leave, sync: true)', async () => {
-  expect.assertions(1);
+  expect.assertions(3);
 
   const leaveError = () => {
     throw new Error('test');
@@ -259,12 +264,14 @@ it('catches error (leave, sync: true)', async () => {
       wrapper,
     });
   } catch (e) {
-    expect(e).toEqual(new Error('Transition error'));
+    expect(e.name).toEqual('BarbaError');
+    expect(e.label).toEqual('Transition error [sync]');
+    expect(e.error).toEqual(new Error('test'));
   }
 });
 
 it('catches error (enter, sync: true)', async () => {
-  expect.assertions(1);
+  expect.assertions(3);
 
   const enterError = () => {
     throw new Error('test');
@@ -279,6 +286,30 @@ it('catches error (enter, sync: true)', async () => {
       wrapper,
     });
   } catch (e) {
-    expect(e).toEqual(new Error('Transition error'));
+    expect(e.name).toEqual('BarbaError');
+    expect(e.label).toEqual('Transition error [sync]');
+    expect(e.error).toEqual(new Error('test'));
+  }
+});
+
+it('catches "global" error (before)', async () => {
+  expect.assertions(2);
+
+  const err = new Error('Test');
+  const beforeError = () => {
+    throw err;
+  };
+  const t = { sync: true, leave() {}, enter() {}, before: beforeError };
+
+  try {
+    await transitions.doPage({
+      data,
+      page,
+      transition: t,
+      wrapper,
+    });
+  } catch (e) {
+    expect(e.name).toEqual('Error');
+    expect(e).toEqual(err);
   }
 });
diff --git a/packages/core/src/core.ts b/packages/core/src/core.ts
index e1e22f12..1077c648 100644
--- a/packages/core/src/core.ts
+++ b/packages/core/src/core.ts
@@ -236,6 +236,7 @@ export class Core {
 
     // 9. Finally, do once…
     this.once(onceData);
+
     // Clean data for first barba transition…
     this._resetData();
   }
@@ -342,15 +343,11 @@ export class Core {
 
     // Check if once transition
     if (this.transitions.hasOnce) {
-      try {
-        const transition = this.transitions.get(readyData, {
-          once: true,
-        }) as ITransitionOnce;
+      const transition = this.transitions.get(readyData, {
+        once: true,
+      }) as ITransitionOnce;
 
-        await this.transitions.doOnce({ transition, data: readyData });
-      } catch (error) {
-        this.logger.error(error);
-      }
+      await this.transitions.doOnce({ transition, data: readyData });
     }
 
     await this.hooks.do('afterEnter', readyData);
@@ -419,7 +416,14 @@ export class Core {
       this._resetData();
     } catch (error) {
       // Something went wrong (rejected promise, error, 404, 505, other…)
-      this.logger.error(error);
+      // TODO: manage / use cases for cancellation
+      // this.logger.debug('Transition cancelled');
+
+      // If no debug mode, force back.
+      /* istanbul ignore else */
+      if (Logger.getLevel() === 0) {
+        this.force(data.current.url.href);
+      }
     }
   }
 
diff --git a/packages/core/src/hooks.ts b/packages/core/src/hooks.ts
index 1e0c47e0..01cc8d76 100644
--- a/packages/core/src/hooks.ts
+++ b/packages/core/src/hooks.ts
@@ -111,7 +111,10 @@ export class Hooks extends HookMethods {
         chain = chain.then(() => runAsync(hook.fn, hook.ctx)(...args));
       });
 
-      return chain;
+      return chain.catch(error => {
+        this.logger.debug(`Hook error [${name}]`);
+        this.logger.error(error);
+      });
     }
 
     return Promise.resolve();
diff --git a/packages/core/src/modules/Error.ts b/packages/core/src/modules/Error.ts
new file mode 100644
index 00000000..6a2ce215
--- /dev/null
+++ b/packages/core/src/modules/Error.ts
@@ -0,0 +1,20 @@
+// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error
+export class BarbaError extends Error {
+  /* istanbul ignore next */
+  constructor(
+    public error: Error,
+    public label = 'Barba error',
+    ...params: any[]
+  ) {
+    // Pass remaining arguments (including vendor specific ones) to parent constructor
+    super(...params);
+
+    // Maintains proper stack trace for where our error was thrown (only available on V8)
+    /* istanbul ignore else */
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, BarbaError);
+    }
+
+    this.name = 'BarbaError';
+  }
+}
diff --git a/packages/core/src/modules/Transitions.ts b/packages/core/src/modules/Transitions.ts
index c51707c4..9a3596d6 100644
--- a/packages/core/src/modules/Transitions.ts
+++ b/packages/core/src/modules/Transitions.ts
@@ -26,6 +26,7 @@ import { hooks } from '../hooks';
 // Utils
 import { dom, helpers, runAsync } from '../utils';
 // Modules
+import { BarbaError } from './Error';
 import { Logger } from './Logger';
 import { Store } from './Store';
 
@@ -107,9 +108,9 @@ export class Transitions {
       await this._doAsyncHook('afterOnce', data, t);
     } catch (error) {
       this._running = false;
+
+      this.logger.debug('Transition error [before/after/once]');
       this.logger.error(error);
-      // TODO: should I throw or should I log…
-      throw new Error('Transition error [once]');
     }
 
     this._running = false;
@@ -179,7 +180,9 @@ export class Transitions {
           await this._doAsyncHook('afterLeave', data, t);
           await this._doAsyncHook('afterEnter', data, t);
         } catch (error) {
-          throw new Error('Transition error [page][sync]');
+          // this.logger.debug('Transition error [sync]');
+          // this.logger.error(error);
+          throw new BarbaError(error, 'Transition error [sync]');
         }
       } else {
         let leaveResult: any = false;
@@ -197,7 +200,9 @@ export class Transitions {
           // TODO: check here "valid" page result
           // before going further
         } catch (error) {
-          throw new Error('Transition error [page][leave]');
+          // this.logger.debug('Transition error [before/after/leave]');
+          // this.logger.error(error);
+          throw new BarbaError(error, 'Transition error [before/after/leave]');
         }
 
         try {
@@ -211,7 +216,9 @@ export class Transitions {
             await this._doAsyncHook('afterEnter', data, t);
           }
         } catch (error) {
-          throw new Error('Transition error [page][enter]');
+          // this.logger.debug('Transition error [before/after/enter]');
+          // this.logger.error(error);
+          throw new BarbaError(error, 'Transition error [before/after/enter]');
         }
       }
 
@@ -221,11 +228,20 @@ export class Transitions {
       await this._doAsyncHook('after', data, t);
     } catch (error) {
       this._running = false;
-      // TODO: use cases for cancellation
+
+      // If "custom/specific" barba error.
+      /* istanbul ignore else */
+      if (error.name && error.name === 'BarbaError') {
+        this.logger.debug(error.label);
+        this.logger.error(error.error);
+
+        throw error;
+      }
+
+      this.logger.debug('Transition error [page]');
       this.logger.error(error);
 
-      // TODO: should I throw or should I log…
-      throw new Error('Transition error');
+      throw error;
     }
 
     this._running = false;
diff --git a/packages/core/src/utils/run-async.ts b/packages/core/src/utils/run-async.ts
index 6c04e434..77ebe60e 100644
--- a/packages/core/src/utils/run-async.ts
+++ b/packages/core/src/utils/run-async.ts
@@ -13,6 +13,7 @@ export function runAsync(
       // Add async to context
       ctx.async = () => {
         async = true;
+
         return (err: any, value: any) => {
           if (err) {
             reject(err);