address feedback

Azure · Feb 10, 2023 · 64ea6c2 · 64ea6c2
1 parent cd0f095
commit 64ea6c2
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 55 deletions.
diff --git a/sdk/core/core-util/src/delay.ts b/sdk/core/core-util/src/delay.ts
@@ -20,52 +20,56 @@ export interface DelayOptions {
 }
 
 /**
- *
- * @param inputs - The inputs for creating an abortable promise are the
- *                 buildPromise function and the cleanupBeforeAbort function.
- *                 buildPromise takes both the resolve and reject functions as
- *                 parameters. cleanupBeforeAbort is called right before the
- *                 promise is rejected.
- * @returns a function that takes an optional DelayOptions parameter and returns
- *          a promise that can be aborted.
+ * Creates an abortable promise.
+ * @param buildPromise - A function that takes the resolve and reject functions as parameters.
+ * @param options - The options for the abortable promise.
+ * @returns A promise that can be aborted.
  * @internal
  */
-export function createAbortablePromise<T>(inputs: {
-  buildPromise: (inputs: {
-    resolve: (value: T | PromiseLike<T>) => void;
-    reject: (reason?: any) => void;
-  }) => void;
-  cleanupBeforeAbort?: () => void;
-}): (options?: DelayOptions) => Promise<T> {
-  const { buildPromise, cleanupBeforeAbort } = inputs;
-  return ({ abortSignal, abortErrorMsg } = {}) =>
-    new Promise((resolve, reject) => {
-      function rejectOnAbort(): void {
-        reject(new AbortError(abortErrorMsg ?? "The operation was aborted."));
-      }
-      function removeListeners(): void {
-        abortSignal?.removeEventListener("abort", onAbort);
-      }
-      function onAbort(): void {
-        cleanupBeforeAbort?.();
-        removeListeners();
-        rejectOnAbort();
-      }
-      if (abortSignal?.aborted) {
-        return rejectOnAbort();
-      }
-      buildPromise({
-        resolve: (x) => {
+export function createAbortablePromise<T>(
+  buildPromise: (
+    resolve: (value: T | PromiseLike<T>) => void,
+    reject: (reason?: any) => void
+  ) => void,
+  options: {
+    cleanupBeforeAbort?: () => void;
+    abortSignal?: AbortSignalLike;
+    abortErrorMsg?: string;
+  }
+): Promise<T> {
+  const { cleanupBeforeAbort, abortSignal, abortErrorMsg } = options;
+  return new Promise((resolve, reject) => {
+    function rejectOnAbort(): void {
+      reject(new AbortError(abortErrorMsg ?? "The operation was aborted."));
+    }
+    function removeListeners(): void {
+      abortSignal?.removeEventListener("abort", onAbort);
+    }
+    function onAbort(): void {
+      cleanupBeforeAbort?.();
+      removeListeners();
+      rejectOnAbort();
+    }
+    if (abortSignal?.aborted) {
+      return rejectOnAbort();
+    }
+    try {
+      buildPromise(
+        (x) => {
           removeListeners();
           resolve(x);
         },
-        reject: (x) => {
+        (x) => {
           removeListeners();
           reject(x);
-        },
-      });
-      abortSignal?.addEventListener("abort", onAbort);
-    });
+        }
+      );
+    } catch (err) {
+      reject(err);
+    }
+
+    abortSignal?.addEventListener("abort", onAbort);
+  });
 }
 
 /**
@@ -77,13 +81,14 @@ export function createAbortablePromise<T>(inputs: {
 export function delay(timeInMs: number, options?: DelayOptions): Promise<void> {
   let token: ReturnType<typeof setTimeout>;
   const { abortSignal, abortErrorMsg } = options || {};
-  return createAbortablePromise<void>({
-    buildPromise: ({ resolve }) => {
+  return createAbortablePromise<void>(
+    (resolve) => {
       token = setTimeout(resolve, timeInMs);
     },
-    cleanupBeforeAbort: () => clearTimeout(token),
-  })({
-    abortSignal,
-    abortErrorMsg: abortErrorMsg ?? StandardAbortMessage,
-  });
+    {
+      cleanupBeforeAbort: () => clearTimeout(token),
+      abortSignal,
+      abortErrorMsg: abortErrorMsg ?? StandardAbortMessage,
+    }
+  );
 }
diff --git a/sdk/core/core-util/test/internal/createAbortablePromise.spec.ts b/sdk/core/core-util/test/internal/createAbortablePromise.spec.ts
@@ -2,7 +2,7 @@
 // Licensed under the MIT license.
 
 import * as sinon from "sinon";
-import { AbortController } from "@azure/abort-controller";
+import { AbortController, AbortSignalLike } from "@azure/abort-controller";
 import chai from "chai";
 import chaiAsPromised from "chai-as-promised";
 import { createAbortablePromise } from "../../src/delay";
@@ -13,12 +13,20 @@ const { assert } = chai;
 describe("createAbortablePromise", function () {
   let token: ReturnType<typeof setTimeout>;
   const delayTime = 2500;
-  const createPromise = createAbortablePromise<void>({
-    buildPromise: ({ resolve }) => {
-      token = setTimeout(resolve, delayTime);
-    },
-    cleanupBeforeAbort: () => clearTimeout(token),
-  });
+  const createPromise = ({
+    abortSignal,
+    abortErrorMsg,
+  }: { abortSignal?: AbortSignalLike; abortErrorMsg?: string } = {}) =>
+    createAbortablePromise<void>(
+      (resolve) => {
+        token = setTimeout(resolve, delayTime);
+      },
+      {
+        cleanupBeforeAbort: () => clearTimeout(token),
+        abortSignal,
+        abortErrorMsg,
+      }
+    );
   afterEach(function () {
     sinon.restore();
   });
@@ -34,12 +42,12 @@ describe("createAbortablePromise", function () {
 
   it("should reject when aborted", async function () {
     const aborter = new AbortController();
-    const abortErrorMsg = "The operation was aborted.";
+    const abortErrorMsg = "The test operation was aborted.";
     const promise = createPromise({
       abortSignal: aborter.signal,
       abortErrorMsg,
     });
     aborter.abort();
-    await assert.isRejected(promise, new RegExp(abortErrorMsg));
+    await assert.isRejected(promise, abortErrorMsg);
   });
 });