refactor: #197 Refactor to use Promises internally (#200)

* rename legacy send method to sendWithCallback * implemented async send wrapper * add async send test suite * update domain example to use async send * prettier run * cleanup * fix test * wip updating expressHandler to use async send * fixing tests * format * replace sendWithCallback to send * wip update docs * cleanup of sendSync * add TODOs for refactor * update README * prettier * add send bind again * cleanup test * clenaup tests * change title * Update lib/raygun.ts Co-authored-by: Sumitra Manga <[email protected]> * wrap HTTP request in Promise * fix: #198 reject promise on error * cleanup * adapt batch transport to transport with Promise * tests are passing * tests are passing * WIP * fix test * fixes and cleanup * add tags to debug loglines * improve logs in raygun.transport * fix: #140 throw error on incoming message too large * improving error logs * error cleanup * package-lock.json changes --------- Co-authored-by: Sumitra Manga <[email protected]>
MindscapeHQ · May 9, 2024 · d8ec309 · d8ec309
1 parent ad719b4
commit d8ec309
Show file tree

Hide file tree

Showing 11 changed files with 186 additions and 172 deletions.
diff --git a/examples/express-sample/package-lock.json b/examples/express-sample/package-lock.json
diff --git a/examples/using-domains/package-lock.json b/examples/using-domains/package-lock.json
diff --git a/lib/raygun.batch.ts b/lib/raygun.batch.ts
@@ -45,18 +45,28 @@ export class RaygunBatchTransport {
 
   /**
    * Enqueues send request to batch processor.
-   * Callback in SendOptions is called when the message is eventually processed.
-   * @param options
+   * @param options send options without callback
+   * @return Promise with response or error if rejected
    */
-  send(options: SendOptions) {
-    this.onIncomingMessage({
-      serializedMessage: options.message,
-      callback: options.callback,
+  send(options: SendOptions): Promise<IncomingMessage> {
+    return new Promise((resolve, reject) => {
+      this.onIncomingMessage({
+        serializedMessage: options.message,
+        // TODO: Switch to using Promises internally
+        // See issue: https://github.com/MindscapeHQ/raygun4node/issues/199
+        callback: (error, message) => {
+          if (error) {
+            reject(error);
+          } else if (message) {
+            resolve(message);
+          }
+        },
+      });
+
+      if (!this.timerId) {
+        this.timerId = setTimeout(() => this.processBatch(), 1000);
+      }
     });
-
-    if (!this.timerId) {
-      this.timerId = setTimeout(() => this.processBatch(), 1000);
-    }
   }
 
   stopProcessing() {
@@ -76,11 +86,9 @@ export class RaygunBatchTransport {
       const messageSize = Math.ceil(messageLength / 1024);
       const startOfMessage = serializedMessage.slice(0, 1000);
 
-      console.warn(
-        `[raygun4node] Error is too large to send to Raygun (${messageSize}kb)\nStart of error: ${startOfMessage}`,
-      );
-
-      return;
+      const errorMessage = `Error is too large to send to Raygun (${messageSize}kb)\nStart of error: ${startOfMessage}`;
+      console.error(`[Raygun4Node] ${errorMessage}`);
+      throw Error(errorMessage);
     }
 
     const messageIsTooLargeToAddToBatch =
@@ -151,6 +159,7 @@ export class RaygunBatchTransport {
       }
 
       // TODO: Callbacks are processed in batch, see how can this be implemented with Promises
+      // See issue: https://github.com/MindscapeHQ/raygun4node/issues/199
       for (const callback of callbacks) {
         if (callback) {
           callVariadicCallback(callback, err, response);
@@ -166,8 +175,15 @@ export class RaygunBatchTransport {
 
     sendBatch({
       message: payload,
-      callback: runAllCallbacks,
       http: this.httpOptions,
-    });
+    })
+      .then((response) => {
+        // Call to original callbacks for success
+        runAllCallbacks(null, response);
+      })
+      .catch((error) => {
+        // Call to original callbacks for error
+        runAllCallbacks(error, null);
+      });
   }
 }
diff --git a/lib/raygun.offline.ts b/lib/raygun.offline.ts
@@ -32,8 +32,9 @@ export class OfflineStorage implements IOfflineStorage {
       path.join(this.cachePath, item),
       "utf8",
       (err, cacheContents) => {
-        // TODO: MessageTransport ignores any errors from the send callback, could this be improved?
+        // Attempt to send stored messages after going online
         this.transport.send(cacheContents);
+        // Ignore result, delete stored content nevertheless
         fs.unlink(path.join(this.cachePath, item), () => {});
       },
     );
@@ -63,13 +64,12 @@ export class OfflineStorage implements IOfflineStorage {
 
     fs.readdir(this.cachePath, (err, files) => {
       if (err) {
-        console.log(`[Raygun4Node] Error reading cache folder`);
-        console.log(err);
+        console.error(`[Raygun4Node] Error reading cache folder`, err);
         return callback(err);
       }
 
       if (files.length > this.cacheLimit) {
-        console.log(`[Raygun4Node] Error cache reached limit`);
+        console.error(`[Raygun4Node] Error cache reached limit`);
         return callback(null);
       }
 
@@ -81,9 +81,7 @@ export class OfflineStorage implements IOfflineStorage {
           return callback(null);
         }
 
-        console.log(`[Raygun4Node] Error writing to cache folder`);
-        console.log(err);
-
+        console.error(`[Raygun4Node] Error writing to cache folder`, err);
         return callback(err);
       });
     });
@@ -98,8 +96,7 @@ export class OfflineStorage implements IOfflineStorage {
   send(callback: (error: Error | null, items?: string[]) => void) {
     this.retrieve((err, items) => {
       if (err) {
-        console.log(`[Raygun4Node] Error reading cache folder`);
-        console.log(err);
+        console.error(`[Raygun4Node] Error reading cache folder`, err);
         return callback(err);
       }
 

diff --git a/lib/raygun.sync.transport.ts b/lib/raygun.sync.transport.ts
@@ -9,11 +9,11 @@
 "use strict";
 
 import { spawnSync } from "child_process";
-import { SendOptionsWithoutCB } from "./types";
+import { SendOptions } from "./types";
 
 const requestProcessSource = require.resolve("./raygun.sync.worker");
 
-function syncRequest(httpOptions: SendOptionsWithoutCB) {
+function syncRequest(httpOptions: SendOptions) {
   const requestProcess = spawnSync("node", [requestProcessSource], {
     input: JSON.stringify(httpOptions),
   });
@@ -27,7 +27,7 @@ function syncRequest(httpOptions: SendOptionsWithoutCB) {
  * Only used to report uncaught exceptions.
  * @param options
  */
-export function send(options: SendOptionsWithoutCB) {
+export function send(options: SendOptions) {
   try {
     syncRequest(options);
   } catch (e) {

diff --git a/lib/raygun.sync.worker.ts b/lib/raygun.sync.worker.ts
@@ -1,22 +1,19 @@
 import fs from "fs";
 import * as transport from "./raygun.transport";
-import { SendOptions, SendOptionsWithoutCB } from "./types";
-import { IncomingMessage } from "http";
+import { SendOptions } from "./types";
 
 // Read stdin synchronously
 const data = fs.readFileSync(0, "utf-8");
 
-const options: SendOptionsWithoutCB = JSON.parse(data);
-const sendOptions: SendOptions = { ...options, callback };
+const options: SendOptions = JSON.parse(data);
 
-transport.send(sendOptions);
-
-function callback(error: Error | null, result: IncomingMessage | null) {
-  if (error) {
-    console.log(`[Raygun4Node] Error sending with sync transport`, error);
-  } else {
+transport
+  .send(options)
+  .then((response) => {
     console.log(
       `[Raygun4Node] Successfully reported uncaught exception to Raygun`,
     );
-  }
-}
+  })
+  .catch((error) => {
+    console.error(`[Raygun4Node] Error sending with sync transport`, error);
+  });
diff --git a/lib/raygun.transport.ts b/lib/raygun.transport.ts
@@ -12,27 +12,27 @@ import http from "http";
 import https from "https";
 
 import { IncomingMessage } from "http";
-import {
-  isCallbackWithError,
-  callVariadicCallback,
-  SendOptions,
-} from "./types";
+import { SendOptions } from "./types";
 
 const API_HOST = "api.raygun.io";
 const DEFAULT_ENDPOINT = "/entries";
 const BATCH_ENDPOINT = "/entries/bulk";
 
-export function sendBatch(options: SendOptions) {
+export function sendBatch(options: SendOptions): Promise<IncomingMessage> {
   return send(options, BATCH_ENDPOINT);
 }
 
-// TODO: Convert this method callbacks to Promise.
 /**
  * Transport implementation that sends error to Raygun.
  * Errors are reported back via callback.
- * @param options
+ * @param options without callback
+ * @param path service endpoint
+ * @return Promise with IncomingMessage or rejected with Error
  */
-export function send(options: SendOptions, path = DEFAULT_ENDPOINT) {
+export function send(
+  options: SendOptions,
+  path = DEFAULT_ENDPOINT,
+): Promise<IncomingMessage> {
   try {
     const data = Buffer.from(options.message);
 
@@ -49,32 +49,33 @@ export function send(options: SendOptions, path = DEFAULT_ENDPOINT) {
       },
     };
 
-    const cb = function (response: IncomingMessage) {
-      if (options.callback) {
-        return callVariadicCallback(options.callback, null, response);
-      }
-    };
+    // Wrap HTTP request in Promise
+    return new Promise((resolve, reject) => {
+      const httpLib = options.http.useSSL ? https : http;
+      const request = httpLib.request(
+        httpOptions,
+        (response: IncomingMessage) => {
+          // request completed successfully
+          resolve(response);
+        },
+      );
 
-    const httpLib = options.http.useSSL ? https : http;
-    const request = httpLib.request(httpOptions, cb);
+      request.on("error", function (e) {
+        console.error(
+          `[Raygun4Node] error ${e.message} occurred while attempting to send error with message: ${options.message}`,
+        );
 
-    request.on("error", function (e) {
-      console.log(
-        `[Raygun4Node] Error ${e.message} occurred while attempting to send error with message: ${options.message}`,
-      );
+        // request failed
+        reject(e);
+      });
 
-      // If the callback has two parameters, it should expect an `error` value.
-      if (options.callback && isCallbackWithError(options.callback)) {
-        options.callback(e, null);
-      }
+      request.write(data);
+      request.end();
     });
-
-    request.write(data);
-    request.end();
   } catch (e) {
-    // TODO: Non-HTTP errors are being ignored, should be better pass them up?
-    console.log(
-      `[Raygun4Node] Error ${e} occurred while attempting to send error with message: ${options.message}`,
+    console.error(
+      `[Raygun4Node] error ${e} occurred while attempting to send error with message: ${options.message}`,
     );
+    return Promise.reject(e);
   }
 }