From 8451e777c023f880912115093f8feb55c93079bf Mon Sep 17 00:00:00 2001 From: Sai Kumar Battinoju Date: Fri, 10 Jan 2025 15:01:39 +0530 Subject: [PATCH] test: address ai bot review comments --- .../services/ErrorHandler/utils.test.ts | 18 ++++++------- .../src/components/core/Analytics.ts | 24 +++++++---------- .../eventRepository/EventRepository.ts | 19 +++----------- .../src/components/utilities/callbacks.ts | 26 +++++++++++++++++++ 4 files changed, 48 insertions(+), 39 deletions(-) create mode 100644 packages/analytics-js/src/components/utilities/callbacks.ts diff --git a/packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts b/packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts index e6b354c5c..4beba5cdd 100644 --- a/packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts +++ b/packages/analytics-js/__tests__/services/ErrorHandler/utils.test.ts @@ -604,16 +604,16 @@ describe('Error Reporting utilities', () => { }); describe('isAllowedToBeNotified', () => { - it('should return true if the error is allowed to be notified', () => { - const result = isAllowedToBeNotified({ message: 'dummy error' } as unknown as Exception); - expect(result).toBeTruthy(); - }); + const testCases = [ + ['dummy error', true, 'should allow generic errors'], + ['The request failed', false, 'should not allow request failures'], + ['', true, 'should allow empty messages'], + ['Network request failed', true, 'should allow network errors'], + ]; - it('should return false if the error is not allowed to be notified', () => { - const result = isAllowedToBeNotified({ - message: 'The request failed', - } as unknown as Exception); - expect(result).toBeFalsy(); + test.each(testCases)('%s -> %s (%s)', (message, expected, testName) => { + const result = isAllowedToBeNotified({ message } as unknown as Exception); + expect(result).toBe(expected); }); }); diff --git a/packages/analytics-js/src/components/core/Analytics.ts b/packages/analytics-js/src/components/core/Analytics.ts index 0130a2a07..97aefb852 100644 --- a/packages/analytics-js/src/components/core/Analytics.ts +++ b/packages/analytics-js/src/components/core/Analytics.ts @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ import { ExternalSrcLoader } from '@rudderstack/analytics-js-common/services/ExternalSrcLoader'; import { batch, effect } from '@preact/signals-core'; -import { isDefined, isFunction, isNull } from '@rudderstack/analytics-js-common/utilities/checks'; +import { isFunction, isNull } from '@rudderstack/analytics-js-common/utilities/checks'; import type { IHttpClient } from '@rudderstack/analytics-js-common/types/HttpClient'; import { clone } from 'ramda'; import type { ILogger } from '@rudderstack/analytics-js-common/types/Logger'; @@ -69,6 +69,7 @@ import { import type { IAnalytics } from './IAnalytics'; import { getConsentManagementData, getValidPostConsentOptions } from '../utilities/consent'; import { dispatchSDKEvent, isDataPlaneUrlValid, isWriteKeyValid } from './utilities'; +import { safelyInvokeCallback } from '../utilities/callbacks'; /* * Analytics class with lifecycle based on state ad user triggered events @@ -322,19 +323,14 @@ class Analytics implements IAnalytics { // Execute onLoaded callback if provided in load options const onLoadedCallbackFn = state.loadOptions.value.onLoaded; - if (isDefined(onLoadedCallbackFn)) { - if (isFunction(onLoadedCallbackFn)) { - // TODO: we need to avoid passing the window object to the callback function - // as this will prevent us from supporting multiple SDK instances in the same page - try { - onLoadedCallbackFn((globalThis as typeof window).rudderanalytics); - } catch (err) { - this.logger.error(CALLBACK_INVOKE_ERROR(LOAD_API), err); - } - } else { - this.logger.error(INVALID_CALLBACK_FN_ERROR(LOAD_API)); - } - } + // TODO: we need to avoid passing the window object to the callback function + // as this will prevent us from supporting multiple SDK instances in the same page + safelyInvokeCallback( + onLoadedCallbackFn, + [(globalThis as typeof window).rudderanalytics], + LOAD_API, + this.logger, + ); // Set lifecycle state batch(() => { diff --git a/packages/analytics-js/src/components/eventRepository/EventRepository.ts b/packages/analytics-js/src/components/eventRepository/EventRepository.ts index fa2345ec8..b2698e5d2 100644 --- a/packages/analytics-js/src/components/eventRepository/EventRepository.ts +++ b/packages/analytics-js/src/components/eventRepository/EventRepository.ts @@ -10,8 +10,6 @@ import type { ApiCallback } from '@rudderstack/analytics-js-common/types/EventAp import { isHybridModeDestination } from '@rudderstack/analytics-js-common/utilities/destinations'; import { API_SUFFIX } from '@rudderstack/analytics-js-common/constants/loggerContexts'; import type { Destination } from '@rudderstack/analytics-js-common/types/Destination'; -import { isDefined, isFunction } from '@rudderstack/analytics-js-common/utilities/checks'; -import { CALLBACK_INVOKE_ERROR, INVALID_CALLBACK_FN_ERROR } from '../../constants/logMessages'; import { state } from '../../state'; import type { IEventRepository } from './types'; import { @@ -20,6 +18,7 @@ import { DMT_EXT_POINT_PREFIX, } from './constants'; import { getFinalEvent, shouldBufferEventsForPreConsent } from './utils'; +import { safelyInvokeCallback } from '../utilities/callbacks'; /** * Event repository class responsible for queuing events for further processing and delivery @@ -170,20 +169,8 @@ class EventRepository implements IEventRepository { ); // Invoke the callback if it exists - if (isDefined(callback)) { - const apiName = `${event.type.charAt(0).toUpperCase()}${event.type.slice(1)}${API_SUFFIX}`; - if (isFunction(callback)) { - try { - // Using the event sent to the data plane queue here - // to ensure the mutated (if any) event is sent to the callback - callback(dpQEvent); - } catch (error) { - this.logger.error(CALLBACK_INVOKE_ERROR(apiName), error); - } - } else { - this.logger.error(INVALID_CALLBACK_FN_ERROR(apiName)); - } - } + const apiName = `${event.type.charAt(0).toUpperCase()}${event.type.slice(1)}${API_SUFFIX}`; + safelyInvokeCallback(callback, [dpQEvent], apiName, this.logger); } } diff --git a/packages/analytics-js/src/components/utilities/callbacks.ts b/packages/analytics-js/src/components/utilities/callbacks.ts new file mode 100644 index 000000000..7d48f27a7 --- /dev/null +++ b/packages/analytics-js/src/components/utilities/callbacks.ts @@ -0,0 +1,26 @@ +import type { ILogger } from '@rudderstack/analytics-js-common/types/Logger'; +import { isDefined, isFunction } from '@rudderstack/analytics-js-common/utilities/checks'; +import { CALLBACK_INVOKE_ERROR, INVALID_CALLBACK_FN_ERROR } from '../../constants/logMessages'; + +const safelyInvokeCallback = ( + callback: unknown, + args: unknown[], + apiName: string, + logger: ILogger, +): void => { + if (!isDefined(callback)) { + return; + } + + if (isFunction(callback)) { + try { + callback(...args); + } catch (error) { + logger.error(CALLBACK_INVOKE_ERROR(apiName), error); + } + } else { + logger.error(INVALID_CALLBACK_FN_ERROR(apiName)); + } +}; + +export { safelyInvokeCallback };