[Breaking change] Fix client ready() method when using async/await syntax [ON HOLD]

CHANGES.txt

@@ -10,6 +10,7 @@
    - Removed internal ponyfills for `Map`, `Set` and `Array.from` global objects, dropping support for IE and other outdated browsers. The SDK now requires the runtime environment to support these features natively or to provide a polyfill.
    - Removed the migration logic for the old format of MySegments keys in LocalStorage introduced in JavaScript SDK v10.17.3.
    - Removed the `sdkClientMethodCSWithTTFactory` function, which handled the logic to bound an optional traffic type to SDK clients. Client-side SDK implementations must use `sdkClientMethodCSWithTT` which, unlike the previous function, does not allow passing a traffic type but simplifies the SDK API.
+   - Bugfixing - Fixed an issue with the client `ready` method that was causing the returned promise to hang on async/await syntax if the promise was rejected. The fix implies a breaking change, since now the user must handle the promise rejection explicitly.
 
 1.17.0 (September 6, 2024)
  - Added `sync.requestOptions.getHeaderOverrides` configuration option to enhance SDK HTTP request Headers for Authorization Frameworks.

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@splitsoftware/splitio-commons",
-  "version": "1.17.1-rc.1",
+  "version": "2.0.0-rc.0",
   "description": "Split JavaScript SDK common components",
   "main": "cjs/index.js",
   "module": "esm/index.js",

src/readiness/__tests__/sdkReadinessManager.spec.ts

@@ -24,6 +24,7 @@ function emitReadyEvent(readinessManager: IReadinessManager) {
   readinessManager.segments.once.mock.calls[0][1]();
   readinessManager.segments.on.mock.calls[0][1]();
   readinessManager.gate.once.mock.calls[0][1]();
+  if (readinessManager.gate.once.mock.calls[3]) readinessManager.gate.once.mock.calls[3][1](); // ready promise
 }
 
 const timeoutErrorMessage = 'Split SDK emitted SDK_READY_TIMED_OUT event.';
@@ -32,6 +33,7 @@ const timeoutErrorMessage = 'Split SDK emitted SDK_READY_TIMED_OUT event.';
 function emitTimeoutEvent(readinessManager: IReadinessManager) {
   readinessManager.gate.once.mock.calls[1][1](timeoutErrorMessage);
   readinessManager.hasTimedout = () => true;
+  if (readinessManager.gate.once.mock.calls[4]) readinessManager.gate.once.mock.calls[4][1](timeoutErrorMessage); // ready promise
 }
 
 describe('SDK Readiness Manager - Event emitter', () => {
@@ -245,8 +247,8 @@ describe('SDK Readiness Manager - Ready promise', () => {
       }
     );
 
-    // any subsequent call to .ready() must be a rejected promise
-    await readyForTimeout.then(
+    // any subsequent call to .ready() must be a rejected promise until the SDK is ready
+    await sdkReadinessManagerForTimedout.sdkStatus.ready().then(
       () => { throw new Error('It should be a promise that was rejected on SDK_READY_TIMED_OUT, not resolved.'); },
       () => {
         expect('A subsequent call should be a rejected promise.');
@@ -258,7 +260,7 @@ describe('SDK Readiness Manager - Ready promise', () => {
     emitReadyEvent(sdkReadinessManagerForTimedout.readinessManager);
 
     // once SDK_READY, `.ready()` returns a resolved promise
-    await ready.then(
+    await sdkReadinessManagerForTimedout.sdkStatus.ready().then(
       () => {
         expect('It should be a resolved promise when the SDK is ready, even after an SDK timeout.');
         loggerMock.mockClear();
@@ -270,57 +272,21 @@ describe('SDK Readiness Manager - Ready promise', () => {
   });
 
   test('Full blown ready promise count as a callback and resolves on SDK_READY', (done) => {
-    const sdkReadinessManager = sdkReadinessManagerFactory(EventEmitterMock, fullSettings);
-    const readyPromise = sdkReadinessManager.sdkStatus.ready();
-
-    // Get the callback
-    const readyEventCB = sdkReadinessManager.readinessManager.gate.once.mock.calls[0][1];
+    let sdkReadinessManager = sdkReadinessManagerFactory(EventEmitterMock, fullSettings);
 
-    readyEventCB();
-    expect(loggerMock.warn).toBeCalledWith(CLIENT_NO_LISTENER); // We would get the warning if the SDK get\'s ready before attaching any callbacks to ready promise.
+    emitReadyEvent(sdkReadinessManager.readinessManager);
+    expect(loggerMock.warn).toBeCalledWith(CLIENT_NO_LISTENER); // We should get a warning if the SDK get's ready before calling the ready method or attaching a listener to the ready event
     loggerMock.warn.mockClear();
 
-    readyPromise.then(() => {
+    sdkReadinessManager = sdkReadinessManagerFactory(EventEmitterMock, fullSettings);
+    sdkReadinessManager.sdkStatus.ready().then(() => {
       expect('The ready promise is resolved when the gate emits SDK_READY.');
       done();
     }, () => {
       throw new Error('This should not be called as the promise is being resolved.');
     });
 
-    readyEventCB();
-    expect(loggerMock.warn).not.toBeCalled(); // But if we have a listener there are no warnings.
-  });
-
-  test('.ready() rejected promises have a default onRejected handler that just logs the error', (done) => {
-    const sdkReadinessManager = sdkReadinessManagerFactory(EventEmitterMock, fullSettings);
-    let readyForTimeout = sdkReadinessManager.sdkStatus.ready();
-
-    emitTimeoutEvent(sdkReadinessManager.readinessManager); // make the SDK "timed out"
-
-    readyForTimeout.then(
-      () => { throw new Error('It should be a promise that was rejected on SDK_READY_TIMED_OUT, not resolved.'); }
-    );
-
-    expect(loggerMock.error).not.toBeCalled(); // not called until promise is rejected
-
-    setTimeout(() => {
-      expect(loggerMock.error.mock.calls).toEqual([[timeoutErrorMessage]]); // If we don\'t handle the rejected promise, an error is logged.
-      readyForTimeout = sdkReadinessManager.sdkStatus.ready();
-
-      setTimeout(() => {
-        expect(loggerMock.error).lastCalledWith('Split SDK has emitted SDK_READY_TIMED_OUT event.'); // If we don\'t handle a new .ready() rejected promise, an error is logged.
-        readyForTimeout = sdkReadinessManager.sdkStatus.ready();
-
-        readyForTimeout
-          .then(() => { throw new Error(); })
-          .then(() => { throw new Error(); })
-          .catch((error) => {
-            expect(error instanceof Error).toBe(true);
-            expect(error.message).toBe('Split SDK has emitted SDK_READY_TIMED_OUT event.');
-            expect(loggerMock.error).toBeCalledTimes(2); // If we provide an onRejected handler, even chaining several onFulfilled handlers, the error is not logged.
-            done();
-          });
-      }, 0);
-    }, 0);
+    emitReadyEvent(sdkReadinessManager.readinessManager);
+    expect(loggerMock.warn).not.toBeCalled(); // But if we have a listener or call the ready method, we get no warnings.
   });
 });

src/readiness/sdkReadinessManager.ts

@@ -1,5 +1,4 @@
 import { objectAssign } from '../utils/lang/objectAssign';
-import { promiseWrapper } from '../utils/promise/wrapper';
 import { readinessManagerFactory } from './readinessManager';
 import { ISdkReadinessManager } from './types';
 import { IEventEmitter, ISettings } from '../types';
@@ -41,33 +40,21 @@ export function sdkReadinessManagerFactory(
   });
 
   /** Ready promise */
-  const readyPromise = generateReadyPromise();
+  let readyPromise: Promise<void>;
 
-  readinessManager.gate.once(SDK_READY_FROM_CACHE, () => {
-    log.info(CLIENT_READY_FROM_CACHE);
-  });
-
-  // default onRejected handler, that just logs the error, if ready promise doesn't have one.
-  function defaultOnRejected(err: any) {
-    log.error(err && err.message);
-  }
-
-  function generateReadyPromise() {
-    const promise = promiseWrapper(new Promise<void>((resolve, reject) => {
-      readinessManager.gate.once(SDK_READY, () => {
-        log.info(CLIENT_READY);
+  readinessManager.gate.once(SDK_READY, () => {
+    log.info(CLIENT_READY);
 
-        if (readyCbCount === internalReadyCbCount && !promise.hasOnFulfilled()) log.warn(CLIENT_NO_LISTENER);
-        resolve();
-      });
-      readinessManager.gate.once(SDK_READY_TIMED_OUT, (message: string) => {
-        reject(new Error(message));
-      });
-    }), defaultOnRejected);
+    if (readyCbCount === internalReadyCbCount && !readyPromise) log.warn(CLIENT_NO_LISTENER);
+  });
 
-    return promise;
-  }
+  readinessManager.gate.once(SDK_READY_TIMED_OUT, (message: string) => {
+    log.error(message);
+  });
 
+  readinessManager.gate.once(SDK_READY_FROM_CACHE, () => {
+    log.info(CLIENT_READY_FROM_CACHE);
+  });
 
   return {
     readinessManager,
@@ -91,34 +78,19 @@ export function sdkReadinessManagerFactory(
           SDK_UPDATE,
           SDK_READY_TIMED_OUT,
         },
-        /**
-         * Returns a promise that will be resolved once the SDK has finished loading (SDK_READY event emitted) or rejected if the SDK has timedout (SDK_READY_TIMED_OUT event emitted).
-         * As it's meant to provide similar flexibility to the event approach, given that the SDK might be eventually ready after a timeout event, calling the `ready` method after the
-         * SDK had timed out will return a new promise that should eventually resolve if the SDK gets ready.
-         *
-         * Caveats: the method was designed to avoid an unhandled Promise rejection if the rejection case is not handled, so that `onRejected` handler is optional when using promises.
-         * However, when using async/await syntax, the rejection should be explicitly propagated like in the following example:
-         * ```
-         * try {
-         *   await client.ready().catch((e) => { throw e; });
-         *   // SDK is ready
-         * } catch(e) {
-         *   // SDK has timedout
-         * }
-         * ```
-         *
-         * @function ready
-         * @returns {Promise<void>}
-         */
         ready() {
-          if (readinessManager.hasTimedout()) {
-            if (!readinessManager.isReady()) {
-              return promiseWrapper(Promise.reject(new Error('Split SDK has emitted SDK_READY_TIMED_OUT event.')), defaultOnRejected);
-            } else {
-              return Promise.resolve();
-            }
-          }
-          return readyPromise;
+          if (readinessManager.isReady()) return Promise.resolve();
+
+          if (readinessManager.hasTimedout()) return Promise.reject(new Error('Split SDK has emitted SDK_READY_TIMED_OUT event.'));
+
+          return readyPromise || (readyPromise = new Promise<void>((resolve, reject) => {
+            readinessManager.gate.once(SDK_READY, () => {
+              resolve();
+            });
+            readinessManager.gate.once(SDK_READY_TIMED_OUT, (message: string) => {
+              reject(new Error(message));
+            });
+          }));
         },
 
         __getStatus() {

src/types.ts

@@ -389,20 +389,9 @@ export interface IStatusInterface extends IEventEmitter {
    */
   Event: EventConsts,
   /**
-   * Returns a promise that will be resolved once the SDK has finished loading (SDK_READY event emitted) or rejected if the SDK has timedout (SDK_READY_TIMED_OUT event emitted).
-   * As it's meant to provide similar flexibility to the event approach, given that the SDK might be eventually ready after a timeout event, calling the `ready` method after the
-   * SDK had timed out will return a new promise that should eventually resolve if the SDK gets ready.
-   *
-   * Caveats: the method was designed to avoid an unhandled Promise rejection if the rejection case is not handled, so that `onRejected` handler is optional when using promises.
-   * However, when using async/await syntax, the rejection should be explicitly propagated like in the following example:
-   * ```
-   * try {
-   *   await client.ready().catch((e) => { throw e; });
-   *   // SDK is ready
-   * } catch(e) {
-   *   // SDK has timedout
-   * }
-   * ```
+   * Returns a promise that resolves once the SDK has finished loading (`SDK_READY` event emitted) or rejected if the SDK has timedout (`SDK_READY_TIMED_OUT` event emitted).
+   * As it's meant to provide similar flexibility to the event approach, given that the SDK might be eventually ready after a timeout event, the `ready` method will return a resolved promise once the SDK is ready.
+   * You must handle the promise rejection to avoid an unhandled promise rejection error, or you can set the `startup.readyTimeout` configuration option to 0 to avoid the timeout and thus the rejection.
    *
    * @function ready
    * @returns {Promise<void>}