From 43d40bd7962bf60ae692fdd47282d278d54b3f2b Mon Sep 17 00:00:00 2001
From: Steven Wu <wusteven815@gmail.com>
Date: Thu, 11 Jan 2024 13:06:25 -0500
Subject: [PATCH] feat: Reject promise immediately if var not found (#1718)

- Implements #1701
- `fetchVariableDefinition` now throws an `Error` when a variable is not
found (instead of throwing `TimeoutError`)
- `TimeoutError` still used and will be thrown on an actual timeout
- Tests added to check for the correct error

---------

Co-authored-by: Mike Bender <mikebender@deephaven.io>
---
 .../jsapi-utils/src/ConnectionUtils.test.ts   | 36 +++++--------------
 packages/jsapi-utils/src/ConnectionUtils.ts   |  8 +++--
 2 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/packages/jsapi-utils/src/ConnectionUtils.test.ts b/packages/jsapi-utils/src/ConnectionUtils.test.ts
index 08d425b847..fca74bb355 100644
--- a/packages/jsapi-utils/src/ConnectionUtils.test.ts
+++ b/packages/jsapi-utils/src/ConnectionUtils.test.ts
@@ -56,38 +56,24 @@ it('finds the right definition if variable exists', async () => {
   expect(mockRemoveListener).toHaveBeenCalled();
 });
 
-it('finds the definition in the second update if not after the first update', async () => {
+it('throws a TimeoutError if finding the variable timed out', async () => {
+  const timeout = 1000;
   const fetchPromise = fetchVariableDefinition(
     connection,
-    testDefinition2.title
+    testDefinition2.title,
+    timeout
   );
 
   expect(mockSubscribeToFieldUpdates).toHaveBeenCalled();
   expect(mockRemoveListener).not.toHaveBeenCalled();
 
-  const listener = mockSubscribeToFieldUpdates.mock.calls[0][0];
-
-  listener({
-    created: [testDefinition1],
-    updated: [],
-    removed: [],
-  });
-
-  expect(mockRemoveListener).not.toHaveBeenCalled();
+  jest.advanceTimersByTime(timeout + 2000);
 
-  listener({
-    created: [testDefinition2],
-    updated: [],
-    removed: [],
-  });
-
-  const result = await fetchPromise;
-
-  expect(result).toBe(testDefinition2);
+  await expect(fetchPromise).rejects.toThrow(TimeoutError);
   expect(mockRemoveListener).toHaveBeenCalled();
 });
 
-it('throws a TimeoutError if variable not found', async () => {
+it('throws an Error if variable not found', async () => {
   const fetchPromise = fetchVariableDefinition(
     connection,
     testDefinition2.title
@@ -104,11 +90,7 @@ it('throws a TimeoutError if variable not found', async () => {
     removed: [],
   });
 
-  expect(mockRemoveListener).not.toHaveBeenCalled();
-
-  jest.runOnlyPendingTimers();
-
+  await expect(fetchPromise).rejects.toThrow(Error);
+  await expect(fetchPromise).rejects.not.toThrow(TimeoutError);
   expect(mockRemoveListener).toHaveBeenCalled();
-
-  await expect(fetchPromise).rejects.toThrow(TimeoutError);
 });
diff --git a/packages/jsapi-utils/src/ConnectionUtils.ts b/packages/jsapi-utils/src/ConnectionUtils.ts
index b8b460219a..72e13a38ab 100644
--- a/packages/jsapi-utils/src/ConnectionUtils.ts
+++ b/packages/jsapi-utils/src/ConnectionUtils.ts
@@ -25,7 +25,7 @@ export function fetchVariableDefinition(
 
     const timeoutId = setTimeout(() => {
       removeListener?.();
-      reject(new TimeoutError(`Variable ${name} not found`));
+      reject(new TimeoutError(`Timeout looking for variable ${name}`));
     }, timeout);
 
     /**
@@ -34,10 +34,12 @@ export function fetchVariableDefinition(
      */
     function handleFieldUpdates(changes: VariableChanges): void {
       const definition = changes.created.find(def => def.title === name);
+      clearTimeout(timeoutId);
+      removeListener?.();
       if (definition != null) {
-        clearTimeout(timeoutId);
-        removeListener?.();
         resolve(definition);
+      } else {
+        reject(new Error(`Variable ${name} not found`));
       }
     }