diff --git a/CHANGES.txt b/CHANGES.txt index a78d749a..037a2d8a 100644 --- a/CHANGES.txt +++ b/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. diff --git a/package-lock.json b/package-lock.json index 2493ec38..9a90a643 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@splitsoftware/splitio-commons", - "version": "1.17.1-rc.1", + "version": "2.0.0-rc.0", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@splitsoftware/splitio-commons", - "version": "1.17.1-rc.1", + "version": "2.0.0-rc.0", "license": "Apache-2.0", "dependencies": { "tslib": "^2.3.1" diff --git a/package.json b/package.json index 1c1a2eb2..f5818af3 100644 --- a/package.json +++ b/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", diff --git a/src/readiness/__tests__/sdkReadinessManager.spec.ts b/src/readiness/__tests__/sdkReadinessManager.spec.ts index 6d639c84..a69ecc97 100644 --- a/src/readiness/__tests__/sdkReadinessManager.spec.ts +++ b/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. }); }); diff --git a/src/readiness/sdkReadinessManager.ts b/src/readiness/sdkReadinessManager.ts index 238297e0..7468cc28 100644 --- a/src/readiness/sdkReadinessManager.ts +++ b/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; - 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((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} - */ 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((resolve, reject) => { + readinessManager.gate.once(SDK_READY, () => { + resolve(); + }); + readinessManager.gate.once(SDK_READY_TIMED_OUT, (message: string) => { + reject(new Error(message)); + }); + })); }, __getStatus() { diff --git a/src/types.ts b/src/types.ts index 3647b804..37cb5d61 100644 --- a/src/types.ts +++ b/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} diff --git a/src/utils/promise/__tests__/wrapper.spec.ts b/src/utils/promise/__tests__/wrapper.spec.ts deleted file mode 100644 index ca0d418a..00000000 --- a/src/utils/promise/__tests__/wrapper.spec.ts +++ /dev/null @@ -1,162 +0,0 @@ -// @ts-nocheck -import { promiseWrapper } from '../wrapper'; - -test('Promise utils / promise wrapper', function (done) { - expect.assertions(58); // number of passHandler, passHandlerFinally, passHandlerWithThrow and `hasOnFulfilled` asserts - - const value = 'value'; - const failHandler = (val) => { done.fail(val); }; - const passHandler = (val) => { expect(val).toBe(value); return val; }; - const passHandlerFinally = (val) => { expect(val).toBeUndefined(); }; - const passHandlerWithThrow = (val) => { expect(val).toBe(value); throw val; }; - const createResolvedPromise = () => new Promise((res) => { setTimeout(() => { res(value); }, 100); }); - const createRejectedPromise = () => new Promise((_, rej) => { setTimeout(() => { rej(value); }, 100); }); - - // resolved promises - let wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(false); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.finally(passHandlerFinally); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler, failHandler).finally(passHandlerFinally); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler).catch(failHandler).finally(passHandlerFinally); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler).catch(failHandler).then(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler).then(passHandler).catch(failHandler).finally(passHandlerFinally).then(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise.then(passHandler).then(passHandlerWithThrow).catch(passHandler).then(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - const wrappedPromise2 = promiseWrapper(createResolvedPromise(), failHandler); - wrappedPromise2.then(() => { - wrappedPromise2.then(passHandler); - }); - expect(wrappedPromise2.hasOnFulfilled()).toBe(true); - - Promise.all([ - promiseWrapper(createResolvedPromise(), failHandler), - promiseWrapper(createResolvedPromise(), failHandler)] - ).then((val) => { expect(val).toEqual([value, value]); }); - - // rejected promises - wrappedPromise = promiseWrapper(createRejectedPromise(), passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(false); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.catch(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(false); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.catch(passHandler).then(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(false); - - // caveat: setting an `onFinally` handler as the first handler, requires an `onRejected` handler if promise is rejected - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.finally(passHandlerFinally).catch(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), passHandler); - wrappedPromise.then(undefined, passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(false); - - wrappedPromise = promiseWrapper(createRejectedPromise(), passHandler); - wrappedPromise.then(failHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.then(failHandler).then(failHandler).catch(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), passHandler); - wrappedPromise.then(failHandler).then(failHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.then(failHandler, passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.then(failHandler).catch(passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.then(failHandler).then(failHandler, passHandler); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - wrappedPromise = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise.then(failHandler).catch(passHandler).then(passHandler).finally(passHandlerFinally); - expect(wrappedPromise.hasOnFulfilled()).toBe(true); - - const wrappedPromise3 = promiseWrapper(createRejectedPromise(), failHandler); - wrappedPromise3.catch(() => { - wrappedPromise3.catch(passHandler); - }); - expect(wrappedPromise3.hasOnFulfilled()).toBe(false); - - Promise.all([ - promiseWrapper(createResolvedPromise(), failHandler), - promiseWrapper(createRejectedPromise(), failHandler)]).catch(passHandler); - - setTimeout(() => { - done(); - }, 1000); - -}); - -test('Promise utils / promise wrapper: async/await', async function () { - - expect.assertions(8); // number of passHandler, passHandlerWithThrow and passHandlerFinally - - const value = 'value'; - const failHandler = (val) => { throw val; }; - const passHandler = (val) => { expect(val).toBe(value); return val; }; - const passHandlerFinally = (val) => { expect(val).toBeUndefined(); }; - const passHandlerWithThrow = (val) => { expect(val).toBe(value); throw val; }; - const createResolvedPromise = () => new Promise((res) => { res(value); }); - const createRejectedPromise = () => new Promise((res, rej) => { rej(value); }); - - try { - const result = await promiseWrapper(createResolvedPromise(), failHandler); - passHandler(result); - } catch (result) { - failHandler(result); - } finally { - passHandlerFinally(); - } - - try { - const result = await promiseWrapper(createRejectedPromise(), failHandler); - failHandler(result); - } catch (result) { - passHandler(result); - } - - let result; - try { - result = await promiseWrapper(createResolvedPromise(), failHandler); - passHandler(result); - passHandlerWithThrow(result); - } catch (error) { - result = passHandler(error); - } finally { - passHandlerFinally(); - } - passHandler(result); -}); diff --git a/src/utils/promise/wrapper.ts b/src/utils/promise/wrapper.ts deleted file mode 100644 index d0100fd6..00000000 --- a/src/utils/promise/wrapper.ts +++ /dev/null @@ -1,60 +0,0 @@ -/** - * wraps a given promise in a new one with a default onRejected function, - * that handles the promise rejection if not other onRejected handler is provided. - * - * Caveats: - * - There are some cases where the `defaultOnRejected` handler is not invoked - * and the promise rejection must be handled by the user (same as the Promise spec): - * - using async/await syntax with a transpiler to Promises - * - setting an `onFinally` handler as the first handler (e.g. `promiseWrapper(Promise.reject()).finally(...)`) - * - setting more than one handler with at least one of them being an onRejected handler - * - If the wrapped promise is rejected when using native async/await syntax, the `defaultOnRejected` handler is invoked - * and neither the catch block nor the remaining try block are executed. - * - * @param customPromise promise to wrap - * @param defaultOnRejected default onRejected function - * @returns a promise that doesn't need to be handled for rejection (except when using async/await syntax) and - * includes a method named `hasOnFulfilled` that returns true if the promise has attached an onFulfilled handler. - */ -export function promiseWrapper(customPromise: Promise, defaultOnRejected: (_: any) => any): Promise & { hasOnFulfilled: () => boolean } { - - let hasOnFulfilled = false; - let hasOnRejected = false; - - function chain(promise: Promise): Promise { - const newPromise: Promise = new Promise((res, rej) => { - return promise.then( - res, - function (value) { - if (hasOnRejected) { - rej(value); - } else { - defaultOnRejected(value); - } - } - ); - }); - - const originalThen = newPromise.then; - - // Using `defineProperty` in case Promise.prototype.then property is not writable - Object.defineProperty(newPromise, 'then', { - value: function (onfulfilled: any, onrejected: any) { - const result: Promise = originalThen.call(newPromise, onfulfilled, onrejected); - if (typeof onfulfilled === 'function') hasOnFulfilled = true; - if (typeof onrejected === 'function') { - hasOnRejected = true; - return result; - } else { - return chain(result); - } - } - }); - - return newPromise; - } - - const result = chain(customPromise) as Promise & { hasOnFulfilled: () => boolean }; - result.hasOnFulfilled = () => hasOnFulfilled; - return result; -}