Don't create always rejecting promises for timeout

Summary:
Our existing `timeout` implementation was always throwing an exception, due to sleeping and then throw an exception, which is than handled but ignored by `Promise.race`.

This implementation has a few problems

1. Because it always throws, having a debugger session with 'break on caught exceptions' will pause on every usage of timeout (rather than just the ones that actually timeout). This makes this way of debugging a bit useless.
2. Throwing exceptions is in principle an expensive process (due to the stack trace generation)
3. Not cancelling the timeout used by sleep is a bit of a waste as well

Reviewed By: lawrencelomax

Differential Revision: D33982717

fbshipit-source-id: d739c02112e1c1bc4cd691af852371d08a99abc6

desktop/flipper-common/src/__tests__/timeout.node.tsx

@@ -0,0 +1,34 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @format
+ */
+
+import {timeout} from '../utils/timeout';
+
+test('resolves', async () => {
+  const p = Promise.resolve(3);
+
+  await expect(timeout(100, p)).resolves.toBe(3);
+});
+
+test('rejects', async () => {
+  const p = Promise.reject(new Error('oops'));
+
+  await expect(timeout(100, p)).rejects.toMatchInlineSnapshot(`[Error: oops]`);
+});
+
+test('times out', async () => {
+  let lateResolve: any;
+  const p = new Promise((r) => {
+    lateResolve = r;
+  });
+
+  await expect(timeout(100, p)).rejects.toMatchInlineSnapshot(
+    `[Error: Timed out in 100 ms.]`,
+  );
+  lateResolve(); // avoid dangling promise after test
+});

desktop/flipper-common/src/utils/timeout.tsx

@@ -7,18 +7,21 @@
  * @format
  */
 
-import {sleep} from './sleep';
-
 export function timeout<T>(
   ms: number,
   promise: Promise<T>,
   timeoutMessage?: string,
 ): Promise<T> {
-  // Create a promise that rejects in <ms> milliseconds
+  return new Promise((resolve, reject) => {
-  const timeout = sleep(ms).then(() => {
+    const timeoutHandle = setTimeout(() => {
-    throw new Error(timeoutMessage || `Timed out in ${ms} ms.`);
+      reject(new Error(timeoutMessage || `Timed out in ${ms} ms.`));
-  });
+    }, ms);
 
-  // Returns a race between our timeout and the passed in promise
+    promise
-  return Promise.race([promise, timeout]);
+      .then(resolve)
+      .finally(() => {
+        clearTimeout(timeoutHandle);
+      })
+      .catch(reject);
+  });
 }

desktop/flipper-ui-core/src/utils/__tests__/promiseTimeout.node.tsx

@@ -9,7 +9,7 @@
 
 import promiseTimeout from '../promiseTimeout';
 
-test('test promiseTimeout for timeout to happen', () => {
+test('test promiseTimeout for timeout to happen', async () => {
   const promise = promiseTimeout(
     200,
     new Promise<void>((resolve) => {
@@ -17,11 +17,10 @@ test('test promiseTimeout for timeout to happen', () => {
         clearTimeout(id);
         resolve();
       }, 500);
-      return 'Executed';
     }),
     'Timed out',
   );
-  return expect(promise).rejects.toThrow('Timed out');
+  await expect(promise).rejects.toThrow('Timed out');
 });
 
 test('test promiseTimeout for timeout not to happen', () => {