fix: Improve checks for Error in onerror handlers (#1468)

Fixes #1466

Author: julienw

lib/application.js

@@ -195,7 +195,13 @@ module.exports = class Application extends Emitter {
    */
 
   onerror(err) {
-    if (!(err instanceof Error)) throw new TypeError(util.format('non-error thrown: %j', err));
+    // When dealing with cross-globals a normal `instanceof` check doesn't work properly.
+    // See https://github.com/koajs/koa/issues/1466
+    // We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
+    const isNativeError =
+      Object.prototype.toString.call(err) === '[object Error]' ||
+      err instanceof Error;
+    if (!isNativeError) throw new TypeError(util.format('non-error thrown: %j', err));
 
     if (404 === err.status || err.expose) return;
     if (this.silent) return;

lib/context.js

@@ -110,7 +110,13 @@ const proto = module.exports = {
     // to node-style callbacks.
     if (null == err) return;
 
-    if (!(err instanceof Error)) err = new Error(util.format('non-error thrown: %j', err));
+    // When dealing with cross-globals a normal `instanceof` check doesn't work properly.
+    // See https://github.com/koajs/koa/issues/1466
+    // We can probably remove it once jest fixes https://github.com/facebook/jest/issues/2549.
+    const isNativeError =
+      Object.prototype.toString.call(err) === '[object Error]' ||
+      err instanceof Error;
+    if (!isNativeError) err = new Error(util.format('non-error thrown: %j', err));
 
     let headerSent = false;
     if (this.headerSent || !this.writable) {

test/application/onerror.js

@@ -16,6 +16,18 @@ describe('app.onerror(err)', () => {
     }, TypeError, 'non-error thrown: foo');
   });
 
+  it('should accept errors coming from other scopes', () => {
+    const ExternError = require('vm').runInNewContext('Error');
+
+    const app = new Koa();
+    const error = Object.assign(new ExternError('boom'), {
+      status: 418,
+      expose: true
+    });
+
+    assert.doesNotThrow(() => app.onerror(error));
+  });
+
   it('should do nothing if status is 404', () => {
     const app = new Koa();
     const err = new Error();

test/context/onerror.js

@@ -1,4 +1,3 @@
-
 'use strict';
 
 const assert = require('assert');
@@ -205,6 +204,40 @@ describe('ctx.onerror(err)', () => {
     });
   });
 
+  describe('when error from another scope thrown', () => {
+    it('should handle it like a normal error', async() => {
+      const ExternError = require('vm').runInNewContext('Error');
+
+      const app = new Koa();
+      const error = Object.assign(new ExternError('boom'), {
+        status: 418,
+        expose: true
+      });
+      app.use((ctx, next) => {
+        throw error;
+      });
+
+      const server = app.listen();
+
+      const gotRightErrorPromise = new Promise((resolve, reject) => {
+        app.on('error', receivedError => {
+          try {
+            assert.strictEqual(receivedError, error);
+            resolve();
+          } catch (e) {
+            reject(e);
+          }
+        });
+      });
+
+      await request(server)
+        .get('/')
+        .expect(418);
+
+      await gotRightErrorPromise;
+    });
+  });
+
   describe('when non-error thrown', () => {
     it('should response non-error thrown message', () => {
       const app = new Koa();
@@ -248,7 +281,7 @@ describe('ctx.onerror(err)', () => {
       });
 
       app.use(async ctx => {
-        throw {key: 'value'}; // eslint-disable-line no-throw-literal
+        throw { key: 'value' }; // eslint-disable-line no-throw-literal
       });
 
       request(app.callback())