feat(Connection): nicer stack traces on protocol errors (puppeteer#1383)

This rewrites protocol errors to have the stack trace of the call site.
mdomke · Dec 9, 2017 · a164524 · a164524
1 parent ea5da00
commit a164524
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 9 deletions.
diff --git a/lib/Connection.js b/lib/Connection.js
@@ -42,7 +42,7 @@ class Connection extends EventEmitter {
     super();
     this._url = url;
     this._lastId = 0;
-    /** @type {!Map<number, {resolve: function, reject: function, method: string}>}*/
+    /** @type {!Map<number, {resolve: function, reject: function, error: !Error, method: string}>}*/
     this._callbacks = new Map();
     this._delay = delay;
 
@@ -71,7 +71,7 @@ class Connection extends EventEmitter {
     debugProtocol('SEND ► ' + message);
     this._ws.send(message);
     return new Promise((resolve, reject) => {
-      this._callbacks.set(id, {resolve, reject, method});
+      this._callbacks.set(id, {resolve, reject, error: new Error(), method});
     });
   }
 
@@ -94,7 +94,7 @@ class Connection extends EventEmitter {
       const callback = this._callbacks.get(object.id);
       this._callbacks.delete(object.id);
       if (object.error)
-        callback.reject(new Error(`Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
+        callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
       else
         callback.resolve(object.result);
     } else {
@@ -121,7 +121,7 @@ class Connection extends EventEmitter {
     }
     this._ws.removeAllListeners();
     for (const callback of this._callbacks.values())
-      callback.reject(new Error(`Protocol error (${callback.method}): Target closed.`));
+      callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): Target closed.`));
     this._callbacks.clear();
     for (const session of this._sessions.values())
       session._onClosed();
@@ -154,7 +154,7 @@ class Session extends EventEmitter {
   constructor(connection, targetId, sessionId) {
     super();
     this._lastId = 0;
-    /** @type {!Map<number, {resolve: function, reject: function, method: string}>}*/
+    /** @type {!Map<number, {resolve: function, reject: function, error: !Error, method: string}>}*/
     this._callbacks = new Map();
     this._connection = connection;
     this._targetId = targetId;
@@ -185,10 +185,10 @@ class Session extends EventEmitter {
         return;
       const callback = this._callbacks.get(id);
       this._callbacks.delete(id);
-      callback.reject(e);
+      callback.reject(rewriteError(callback.error, e && e.message));
     });
     return new Promise((resolve, reject) => {
-      this._callbacks.set(id, {resolve, reject, method});
+      this._callbacks.set(id, {resolve, reject, error: new Error(), method});
     });
   }
 
@@ -202,7 +202,7 @@ class Session extends EventEmitter {
       const callback = this._callbacks.get(object.id);
       this._callbacks.delete(object.id);
       if (object.error)
-        callback.reject(new Error(`Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
+        callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
       else
         callback.resolve(object.result);
     } else {
@@ -218,10 +218,20 @@ class Session extends EventEmitter {
 
   _onClosed() {
     for (const callback of this._callbacks.values())
-      callback.reject(new Error(`Protocol error (${callback.method}): Target closed.`));
+      callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): Target closed.`));
     this._callbacks.clear();
     this._connection = null;
   }
 }
 
+/**
+ * @param {!Error} error
+ * @param {string} message
+ * @return {!Error}
+ */
+function rewriteError(error, message) {
+  error.message = message;
+  return error;
+}
+
 module.exports = {Connection, Session};
diff --git a/test/test.js b/test/test.js
@@ -3271,6 +3271,17 @@ describe('Page', function() {
       await newPage.close();
     }));
   });
+
+  describe('Connection', function() {
+    it('should throw nice errors', SX(async function() {
+      const error = await theSourceOfTheProblems().catch(error => error);
+      expect(error.stack).toContain('theSourceOfTheProblems');
+      expect(error.message).toContain('ThisCommand.DoesNotExist');
+      async function theSourceOfTheProblems() {
+        await page._client.send('ThisCommand.DoesNotExist');
+      }
+    }));
+  });
 });
 
 if (process.env.COVERAGE) {