feat: add error handling to all public apis #1907
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes several modifications across various files in the analytics-js-common package. Key changes involve renaming and replacing constants, updating type definitions to include Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1907 +/- ##
===========================================
+ Coverage 57.47% 57.69% +0.22%
===========================================
Files 485 485
Lines 16495 16569 +74
Branches 3304 3325 +21
===========================================
+ Hits 9481 9560 +79
+ Misses 5763 5740 -23
- Partials 1251 1269 +18 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (20)
packages/analytics-js-common/src/types/ApiObject.ts (1)
13-14: Consider adding JSDoc documentation for the array type.To improve code maintainability and help API consumers, consider documenting the array type's behavior with undefined values.
export type ApiObject = { [index: string]: | string | number | boolean | ApiObject | null | Date + /** + * Array of values that can be undefined. + * Consumers should handle potential undefined values when accessing array elements. + */ | (string | number | boolean | null | Date | ApiObject | undefined)[] | undefined; };packages/analytics-js-common/src/constants/loggerContexts.ts (1)
14-14: Consider using a more descriptive constant name.While
RSAis more concise, it might not be immediately clear what it represents. Consider using a more descriptive name likeRUDDERSTACK_ANALYTICS_CONTEXTto maintain clarity and self-documentation.-const RSA = 'RudderStackAnalytics'; +const RUDDERSTACK_ANALYTICS_CONTEXT = 'RudderStackAnalytics'; export { // ... other exports ... - RSA, + RUDDERSTACK_ANALYTICS_CONTEXT, ANALYTICS_CORE, };Also applies to: 31-31
packages/analytics-js-common/src/types/IRudderAnalytics.ts (1)
191-191: Consider simplifying the return type to avoid triple state.The current return type
Nullable<number> | undefinedcreates a triple state (null, undefined, number) which can complicate error handling for consumers. Consider using eitherNullable<number>ornumber | undefinedto maintain a simpler dual state.If the intention is to represent "no session", choose one of:
// Option 1: Use null to represent "no session" getSessionId(): Nullable<number>; // Option 2: Use undefined to represent "no session" getSessionId(): number | undefined;packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (2)
181-206: Consider refactoring common sanitization patterns.There's significant code duplication in the sanitization and type assertion logic across all methods. Consider extracting common patterns into helper functions:
- A helper for sanitizing and type-asserting parameters
- A helper for handling function arguments in different positions
This would improve maintainability and reduce the risk of inconsistencies.
Example helper function:
type SanitizeAndAssertConfig<T> = { value: unknown; expectedType: string; defaultValue?: T; }; function sanitizeAndAssertType<T>({ value, expectedType, defaultValue }: SanitizeAndAssertConfig<T>): T { const sanitized = getSanitizedValue(value); // Add appropriate type checks based on expectedType // Return sanitized value with correct type assertion or defaultValue }Also applies to: 232-259, 303-330, 368-395
Line range hint
60-407: Enhance error handling to meet PR objectives.While input sanitization is a good start, consider adding explicit error handling to align better with the PR's objective of "adding error handling to all public APIs":
- Add try-catch blocks around operations that might fail (e.g.,
clone,mergeDeepRight)- Define and handle specific error cases (e.g., invalid input types)
- Consider adding error logging or telemetry for debugging
Example enhancement:
function safeClone<T>(value: T): T { try { return clone(value); } catch (error) { // Log error for debugging // Return a safe default or rethrow with more context throw new Error(`Failed to clone value: ${error.message}`); } }packages/analytics-js/src/constants/logMessages.ts (1)
78-80: Consider enhancing the error message format.While the implementation follows the established pattern, consider extracting more details from the Error object for better debugging:
-const GLOBAL_UNHANDLED_ERROR = (context: string, error: Error): string => - `${context}${LOG_CONTEXT_SEPARATOR}An unhandled error occurred: ${error}`; +const GLOBAL_UNHANDLED_ERROR = (context: string, error: Error): string => + `${context}${LOG_CONTEXT_SEPARATOR}An unhandled error occurred: ${error.message}${ + error.stack ? `\nStack trace: ${error.stack}` : '' + }`;This would provide more detailed error information when available, making debugging easier.
packages/analytics-js/src/app/RudderAnalytics.ts (1)
532-536: Add error handling to getter methods for consistencyFor consistency with other public methods and to handle any potential errors, consider adding
try-catchblocks to the getter methodsgetUserId,getUserTraits,getGroupId, andgetGroupTraits. This ensures that any unexpected errors are properly caught and logged.Apply this diff to add error handling to
getUserId:- return this.getAnalyticsInstance()?.getUserId(); + try { + return this.getAnalyticsInstance()?.getUserId(); + } catch (error: any) { + this.logger.error(GLOBAL_UNHANDLED_ERROR(RSA, error)); + return undefined; + }Repeat similar changes for the other getter methods.
Also applies to: 540-544, 548-552, 556-560
packages/analytics-js/src/components/core/Analytics.ts (13)
108-143: Ensure new error handling inloadmethod is covered by unit testsThe added try-catch block in the
loadmethod improves robustness by handling exceptions. However, static analysis indicates that these lines are not covered by unit tests. Please add tests to cover these scenarios to maintain code coverage and ensure reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 110-110: packages/analytics-js/src/components/core/Analytics.ts#L110
Added line #L110 was not covered by tests
[warning] 142-142: packages/analytics-js/src/components/core/Analytics.ts#L142
Added line #L142 was not covered by tests
187-187: Add unit tests for error handling instartLifecycleThe error handling added in line 187 captures exceptions during the lifecycle management. To ensure this critical path is tested, please include unit tests that simulate errors in
startLifecycleand verify that they are handled correctly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 187-187: packages/analytics-js/src/components/core/Analytics.ts#L187
Added line #L187 was not covered by tests
Line range hint
447-903: Refactor repetitive try-catch blocks to reduce code duplicationMultiple methods (
ready,page,track,identify,alias,group,reset,getAnonymousId,setAnonymousId,getUserId,getUserTraits,getGroupId,getGroupTraits,startSession,endSession,getSessionId,consent,setAuthToken) now include similar try-catch blocks for error handling. This repetition can be refactored to enhance maintainability.Consider creating a higher-order function to wrap these methods:
private handleErrors<T>(fn: () => T): T | undefined { try { return fn(); } catch (err: any) { this.errorHandler.onError(err, ANALYTICS_CORE); return undefined; } }Then, refactor your methods like so:
-ready(callback: ApiCallback, isBufferedInvocation = false) { - try { - const type = 'ready'; - // Method implementation... - } catch (err: any) { - this.errorHandler.onError(err, ANALYTICS_CORE); - } +ready(callback: ApiCallback, isBufferedInvocation = false) { + return this.handleErrors(() => { + const type = 'ready'; + // Method implementation... + }); }This approach centralizes error handling and reduces redundancy.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 474-474: packages/analytics-js/src/components/core/Analytics.ts#L474
Added line #L474 was not covered by tests
[warning] 483-483: packages/analytics-js/src/components/core/Analytics.ts#L483
Added line #L483 was not covered by tests
[warning] 519-519: packages/analytics-js/src/components/core/Analytics.ts#L519
Added line #L519 was not covered by tests
[warning] 533-533: packages/analytics-js/src/components/core/Analytics.ts#L533
Added line #L533 was not covered by tests
[warning] 560-560: packages/analytics-js/src/components/core/Analytics.ts#L560
Added line #L560 was not covered by tests
[warning] 603-603: packages/analytics-js/src/components/core/Analytics.ts#L603
Added line #L603 was not covered by tests
[warning] 635-635: packages/analytics-js/src/components/core/Analytics.ts#L635
Added line #L635 was not covered by tests
[warning] 669-669: packages/analytics-js/src/components/core/Analytics.ts#L669
Added line #L669 was not covered by tests
[warning] 690-690: packages/analytics-js/src/components/core/Analytics.ts#L690
Added line #L690 was not covered by tests
[warning] 695-695: packages/analytics-js/src/components/core/Analytics.ts#L695
Added line #L695 was not covered by tests
474-474: Include unit tests for callback error handling inreadymethodLine 474 handles exceptions thrown by the
callbackfunction. To ensure this path is tested, add unit tests that pass a faulty callback and verify that the error is properly captured byerrorHandler.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 474-474: packages/analytics-js/src/components/core/Analytics.ts#L474
Added line #L474 was not covered by tests
483-483: Ensurereadymethod's error handling is testedThe catch block at line 483 captures errors in the
readymethod. Adding unit tests for this scenario will improve code coverage and reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 483-483: packages/analytics-js/src/components/core/Analytics.ts#L483
Added line #L483 was not covered by tests
519-531: Add unit tests for ad-blocked page event logicThe code handling ad-block detection (lines 519-531) is not covered by tests according to static analysis. Please include tests to verify that the ad-block page event is triggered appropriately when ad blockers are detected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 519-519: packages/analytics-js/src/components/core/Analytics.ts#L519
Added line #L519 was not covered by tests
533-533: Test error handling inpagemethodLine 533 captures exceptions in the
pagemethod. Adding unit tests that simulate errors will ensure this error handling path is functioning correctly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 533-533: packages/analytics-js/src/components/core/Analytics.ts#L533
Added line #L533 was not covered by tests
560-560: Increase test coverage fortrackmethod error handlingThe catch block at line 560 in the
trackmethod is not covered by tests. Please add unit tests to validate this error handling.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 560-560: packages/analytics-js/src/components/core/Analytics.ts#L560
Added line #L560 was not covered by tests
603-603: Ensure error handling inidentifymethod is testedLine 603 handles errors in the
identifymethod. Including unit tests will help ensure that exceptions are correctly managed.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 603-603: packages/analytics-js/src/components/core/Analytics.ts#L603
Added line #L603 was not covered by tests
635-635: Test error handling inaliasmethodThe catch block at line 635 is not covered by tests. Adding tests for exception scenarios in the
aliasmethod will enhance reliability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 635-635: packages/analytics-js/src/components/core/Analytics.ts#L635
Added line #L635 was not covered by tests
669-669: Add unit tests forgroupmethod error handlingLine 669 captures errors in the
groupmethod. Please include unit tests to cover this code path.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 669-669: packages/analytics-js/src/components/core/Analytics.ts#L669
Added line #L669 was not covered by tests
690-690: Increase test coverage forresetmethodThe error handling in line 690 is not currently tested. Adding unit tests will ensure that the
resetmethod behaves correctly under error conditions.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 690-690: packages/analytics-js/src/components/core/Analytics.ts#L690
Added line #L690 was not covered by tests
695-695: Verify return value ofgetAnonymousIdand add testsLine 695 introduces a try-catch in
getAnonymousId. Ensure that the method returnsundefinedwhen an error occurs and add tests to cover this scenario.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 695-695: packages/analytics-js/src/components/core/Analytics.ts#L695
Added line #L695 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/analytics-js-common/src/constants/loggerContexts.ts(2 hunks)packages/analytics-js-common/src/types/ApiObject.ts(1 hunks)packages/analytics-js-common/src/types/IRudderAnalytics.ts(2 hunks)packages/analytics-js-common/src/utilities/eventMethodOverloads.ts(6 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(14 hunks)packages/analytics-js/src/components/core/Analytics.ts(4 hunks)packages/analytics-js/src/constants/logMessages.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1902
File: packages/analytics-js-common/src/utilities/sanitize.ts:0-0
Timestamp: 2024-10-28T08:02:55.044Z
Learning: The previous suggestions on the `getSanitizedValue` function in `packages/analytics-js-common/src/utilities/sanitize.ts` are no longer valid.
🪛 Biome
packages/analytics-js/src/app/RudderAnalytics.ts
[error] 68-68: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🪛 GitHub Check: codecov/patch
packages/analytics-js/src/app/RudderAnalytics.ts
[warning] 108-108: packages/analytics-js/src/app/RudderAnalytics.ts#L108
Added line #L108 was not covered by tests
[warning] 123-123: packages/analytics-js/src/app/RudderAnalytics.ts#L123
Added line #L123 was not covered by tests
[warning] 145-146: packages/analytics-js/src/app/RudderAnalytics.ts#L145-L146
Added lines #L145 - L146 were not covered by tests
[warning] 181-181: packages/analytics-js/src/app/RudderAnalytics.ts#L181
Added line #L181 was not covered by tests
[warning] 331-331: packages/analytics-js/src/app/RudderAnalytics.ts#L331
Added line #L331 was not covered by tests
[warning] 380-380: packages/analytics-js/src/app/RudderAnalytics.ts#L380
Added line #L380 was not covered by tests
[warning] 407-407: packages/analytics-js/src/app/RudderAnalytics.ts#L407
Added line #L407 was not covered by tests
[warning] 440-440: packages/analytics-js/src/app/RudderAnalytics.ts#L440
Added line #L440 was not covered by tests
[warning] 461-461: packages/analytics-js/src/app/RudderAnalytics.ts#L461
Added line #L461 was not covered by tests
[warning] 491-492: packages/analytics-js/src/app/RudderAnalytics.ts#L491-L492
Added lines #L491 - L492 were not covered by tests
[warning] 499-499: packages/analytics-js/src/app/RudderAnalytics.ts#L499
Added line #L499 was not covered by tests
[warning] 507-507: packages/analytics-js/src/app/RudderAnalytics.ts#L507
Added line #L507 was not covered by tests
[warning] 515-516: packages/analytics-js/src/app/RudderAnalytics.ts#L515-L516
Added lines #L515 - L516 were not covered by tests
[warning] 527-527: packages/analytics-js/src/app/RudderAnalytics.ts#L527
Added line #L527 was not covered by tests
[warning] 567-567: packages/analytics-js/src/app/RudderAnalytics.ts#L567
Added line #L567 was not covered by tests
[warning] 591-591: packages/analytics-js/src/app/RudderAnalytics.ts#L591
Added line #L591 was not covered by tests
[warning] 599-599: packages/analytics-js/src/app/RudderAnalytics.ts#L599
Added line #L599 was not covered by tests
packages/analytics-js/src/components/core/Analytics.ts
[warning] 110-110: packages/analytics-js/src/components/core/Analytics.ts#L110
Added line #L110 was not covered by tests
[warning] 142-142: packages/analytics-js/src/components/core/Analytics.ts#L142
Added line #L142 was not covered by tests
[warning] 187-187: packages/analytics-js/src/components/core/Analytics.ts#L187
Added line #L187 was not covered by tests
[warning] 474-474: packages/analytics-js/src/components/core/Analytics.ts#L474
Added line #L474 was not covered by tests
[warning] 483-483: packages/analytics-js/src/components/core/Analytics.ts#L483
Added line #L483 was not covered by tests
[warning] 519-519: packages/analytics-js/src/components/core/Analytics.ts#L519
Added line #L519 was not covered by tests
[warning] 533-533: packages/analytics-js/src/components/core/Analytics.ts#L533
Added line #L533 was not covered by tests
[warning] 560-560: packages/analytics-js/src/components/core/Analytics.ts#L560
Added line #L560 was not covered by tests
[warning] 603-603: packages/analytics-js/src/components/core/Analytics.ts#L603
Added line #L603 was not covered by tests
[warning] 635-635: packages/analytics-js/src/components/core/Analytics.ts#L635
Added line #L635 was not covered by tests
[warning] 669-669: packages/analytics-js/src/components/core/Analytics.ts#L669
Added line #L669 was not covered by tests
[warning] 690-690: packages/analytics-js/src/components/core/Analytics.ts#L690
Added line #L690 was not covered by tests
[warning] 695-695: packages/analytics-js/src/components/core/Analytics.ts#L695
Added line #L695 was not covered by tests
🔇 Additional comments (8)
packages/analytics-js-common/src/types/ApiObject.ts (1)
13-14: LGTM, but note this is a breaking change.
The addition of undefined to the array type definition improves type safety and aligns with the PR's error handling objectives. However, this is a breaking change that might affect existing consumers of the API.
Let's verify the impact:
packages/analytics-js-common/src/constants/loggerContexts.ts (1)
14-14: Verify all usages of the renamed constant.
The constant RS_APP has been renamed to RSA with an updated value. This is a breaking change that could affect logging and error handling across the codebase.
Let's verify all occurrences to ensure consistent updates:
✅ Verification successful
The constant renaming from RS_APP to RSA appears to be a safe change.
The search results show that:
- The old
RS_APPconstant is only present in the Chrome extension example code which appears to be legacy/example code. - The new
RSAconstant is properly used across the main codebase:- Imported and used in error handling in RudderAnalytics.ts
- Used in SDK event names (
RSA_Initialised,RSA_Ready) - Referenced in test files and examples
The change is part of a broader error handling improvement and the new name aligns well with its usage in the analytics core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of RS_APP and verify RSA usage
# Test 1: Search for any remaining RS_APP references that might have been missed
echo "Checking for any remaining RS_APP references..."
rg "RS_APP"
# Test 2: Verify the new RSA constant usage
echo "Checking RSA usage..."
rg "RSA" -A 2
Length of output: 14214
packages/analytics-js/.size-limit.mjs (2)
98-98: Verify size increase and obtain CODEOWNERS approval.
The size limit increase of 0.5 KiB for "Core (Content Script) - Legacy - NPM (CJS)" appears justified for error handling additions. However, as per the file header comment, please ensure you have contacted CODEOWNERS for approval of this change.
27-27: Verify size increase and obtain CODEOWNERS approval.
The size limit increase of 0.5 KiB for "Core - Legacy - CDN" seems reasonable given the addition of error handling. However, as per the file header comment, please ensure you have contacted CODEOWNERS for approval of this change.
packages/analytics-js-common/src/types/IRudderAnalytics.ts (1)
88-88: LGTM! Improved error handling for analytics instance retrieval.
The change to return T | undefined explicitly handles the case where no analytics instance exists for the provided writeKey, making the API more predictable and safer to use.
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (2)
9-9: LGTM! Consistent input sanitization approach.
The addition of getSanitizedValue from the JSON utilities and its consistent application across all method parameters aligns well with the PR's objective of improving error handling for public APIs.
60-70: Verify type safety of assertions after sanitization.
The type assertions after sanitization (e.g., as string, as Nullable<ApiObject>) might be unsafe if getSanitizedValue modifies the type of the input value. Consider adding runtime type checks before the assertions or documenting the guarantees provided by getSanitizedValue.
✅ Verification successful
Type assertions after sanitization are safe
Based on the implementation and tests found in packages/analytics-js-common/__tests__/utilities/json.test.ts, getSanitizedValue is a type-preserving function that:
- Returns the input value unchanged for primitive types (string, number, null, undefined)
- Only transforms BigInt values to '[BigInt]' string
- Handles circular references by replacing them with '[Circular Reference]' string
- Preserves the structure of objects and arrays
The type assertions in the code are safe because:
- String inputs remain strings after sanitization
- Objects remain objects with the same shape
- The function is explicitly typed to preserve the input type
<T = any>(value: T): T
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getSanitizedValue implementation and type definition
ast-grep --pattern 'function getSanitizedValue($_) {
$$$
}'
# Search for any type-related tests
rg -l "getSanitizedValue.*test" -g "*.test.*"
Length of output: 111
Script:
#!/bin/bash
# Let's try different approaches to find getSanitizedValue
# Search for getSanitizedValue function with ripgrep
rg -A 5 "getSanitizedValue"
# Search for files containing getSanitizedValue
rg -l "getSanitizedValue"
# Search for any exports of getSanitizedValue
ast-grep --pattern 'export $_ getSanitizedValue'
# Search for type tests with a broader pattern
rg -l "sanitized.*test" -g "*.test.*"
Length of output: 14930
packages/analytics-js/src/constants/logMessages.ts (1)
327-327: LGTM!
The export is properly placed and follows the file's alphabetical ordering convention.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
packages/analytics-js-common/src/utilities/errors.ts (1)
20-24: Consider standardizing error event handling patternsAs this is part of a larger effort to add error handling to all public APIs, consider:
- Documenting standard error handling patterns and when to use this utility
- Defining a consistent error event schema for better error tracking
- Adding error categorization/severity levels for better error management
Would you like me to help create documentation templates or example usage patterns?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-21: packages/analytics-js-common/src/utilities/errors.ts#L21
Added line #L21 was not covered by testspackages/analytics-js/src/app/RudderAnalytics.ts (2)
61-105: Consider enhancing error handling in the constructor.While the error handling is good, consider re-throwing the error after dispatching it, as constructor failures should prevent instance creation:
} catch (error: any) { dispatchErrorEvent(error); + throw error; // Prevent instance creation on critical initialization errors }🧰 Tools
🪛 Biome
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
Line range hint
237-251: Add input validation for page lifecycle events.Consider validating the
eventsarray to ensure it only contains validPageLifecycleEventsvalues:trackPageLoadedEvent( events: PageLifecycleEvents[], options: ApiOptions, preloadedEventsArray: PreloadedEventCall[], ) { + const validEvents = events.filter(event => + Object.values(PageLifecycleEvents).includes(event) + ); - if (events.length === 0 || events.includes(PageLifecycleEvents.LOADED)) { + if (validEvents.length === 0 || validEvents.includes(PageLifecycleEvents.LOADED)) {packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (1)
334-354: Refactor Repetitive Error Handling LogicMultiple tests mock
getAnalyticsInstanceto throw an error and assert that an error event is dispatched. Consider refactoring this repetitive pattern to improve maintainability.You can extract the common setup and teardown into
beforeEachandafterEachhooks or helper functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/analytics-js-common/src/utilities/errors.ts(1 hunks)packages/analytics-js/.size-limit.mjs(3 hunks)packages/analytics-js/__tests__/app/RudderAnalytics.test.ts(4 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(15 hunks)packages/analytics-js/src/constants/logMessages.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/analytics-js/src/constants/logMessages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/.size-limit.mjs
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/src/app/RudderAnalytics.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1907
File: packages/analytics-js/src/app/RudderAnalytics.ts:68-68
Timestamp: 2024-11-07T05:29:11.813Z
Learning: In `packages/analytics-js/src/app/RudderAnalytics.ts`, the constructor of the `RudderAnalytics` class intentionally returns `RudderAnalytics.globalSingleton` to implement a singleton pattern. In JavaScript, returning an object from a constructor causes the `new` operator to return that object instead of a new instance. Therefore, when reviewing, avoid flagging returns from constructors as issues if they are intended for singleton implementations.
🪛 GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/errors.ts
[warning] 21-21: packages/analytics-js-common/src/utilities/errors.ts#L21
Added line #L21 was not covered by tests
🪛 Biome
packages/analytics-js/src/app/RudderAnalytics.ts
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🔇 Additional comments (9)
packages/analytics-js-common/src/utilities/errors.ts (2)
24-24: LGTM!
The export statement is correctly updated to include the new function.
20-22: 🛠️ Refactor suggestion
Add error handling, type checking, and documentation
The dispatchErrorEvent function needs several improvements for robustness:
- Add error handling for non-browser environments
- Add type checking for the error parameter
- Add JSDoc documentation
Consider this implementation:
+/**
+ * Dispatches an error event to the global context
+ * @param error The error to be dispatched
+ * @throws {Error} If dispatchEvent is not available in the current environment
+ */
-const dispatchErrorEvent = (error: any) => {
- (globalThis as typeof window).dispatchEvent(new ErrorEvent('error', { error }));
-};
+const dispatchErrorEvent = (error: unknown): void => {
+ if (typeof globalThis.dispatchEvent !== 'function') {
+ throw new Error('dispatchEvent is not available in the current environment');
+ }
+
+ const errorToDispatch = isTypeOfError(error) ? error : new Error(stringifyData(error));
+ (globalThis as typeof window).dispatchEvent(new ErrorEvent('error', { error: errorToDispatch }));
+};Let's verify the test coverage:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-21: packages/analytics-js-common/src/utilities/errors.ts#L21
Added line #L21 was not covered by tests
packages/analytics-js/src/app/RudderAnalytics.ts (2)
20-20: LGTM! Import changes align with error handling objectives.
The new imports for error handling and JSON sanitization support the PR's goal of implementing robust error handling across public APIs.
Also applies to: 26-27
372-378: LGTM! Consistent error handling pattern.
The implementation follows a consistent pattern across all methods:
- Try-catch block wrapping the method body
- Input sanitization using
getSanitizedValue - Error dispatching using
dispatchErrorEvent
Also applies to: 399-405, 432-438, 455-459, 486-491, 496-500, 503-509, 512-520, 555-560, 563-568, 579-584, 587-592
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (5)
58-63: Correct Handling of Undefined globalThis.rudderanalytics
The test case correctly verifies that getPreloadedEvents() returns an empty array when globalThis.rudderanalytics is undefined, ensuring robustness against unexpected global states.
65-77: Accurate Retrieval of Buffered Events
This test ensures that when globalThis.rudderanalytics is an array, getPreloadedEvents() correctly returns the buffered events, validating the proper handling of preloaded events.
250-271: Mismatch Between Thrown and Expected Error Types
Similar to the previous comment, the mocked getAnalyticsInstance throws an Error, but the test expects a different error type or message.
306-326: Consistent Error Assertions
Ensure that the error being thrown in the mock and the error expected in the expect statement are the same. This applies to tests where getAnalyticsInstance is mocked to throw an error.
Line range hint 605-710: Comprehensive Tests for trackPageLifecycleEvents
The tests for the trackPageLifecycleEvents method are thorough and cover various scenarios, including different configurations of the autoTrack and useBeacon options. The simulation of the 'beforeunload' event is correctly implemented, ensuring that the Page Unloaded event is tracked appropriately.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/analytics-js/src/app/RudderAnalytics.ts (2)
154-179: Consider enhancing error handling in load method.While the error handling is good, the method could benefit from additional error checks:
- Validate writeKey and dataPlaneUrl before proceeding
- Handle potential errors during preloaded events processing
load(writeKey: string, dataPlaneUrl: string, loadOptions?: Partial<LoadOptions>): void { try { + if (!writeKey || !dataPlaneUrl) { + throw new Error('writeKey and dataPlaneUrl are required'); + } + if (this.analyticsInstances[writeKey]) { return; }
584-597: Consider adding input validation for auth token and consent options.While error handling is implemented correctly, consider adding validation for the auth token and consent options before processing:
setAuthToken(token: string): void { try { + if (!token || typeof token !== 'string') { + throw new Error('Invalid auth token'); + } this.getAnalyticsInstance()?.setAuthToken(getSanitizedValue(token)); } catch (error: any) { dispatchErrorEvent(error); } }packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (2)
Line range hint
86-840: Consider refactoring repetitive error handling test patternsMultiple test cases follow the same pattern of:
- Setting up a dispatch event spy
- Mocking getAnalyticsInstance to throw an error
- Calling the method
- Asserting the error event
- Cleaning up spies
Consider creating a test helper function to reduce code duplication.
Example helper function:
const testErrorHandling = ( methodName: string, methodCall: () => void, expectedError: Error ) => { const dispatchEventSpy = jest.spyOn(window, 'dispatchEvent'); const getAnalyticsInstanceSpy = jest .spyOn(rudderAnalytics, 'getAnalyticsInstance') .mockImplementation(() => { throw expectedError; }); methodCall(); expect(dispatchEventSpy).toHaveBeenCalledWith( new ErrorEvent('error', { error: expectedError, }), ); dispatchEventSpy.mockRestore(); getAnalyticsInstanceSpy.mockRestore(); };
476-493: Consider improving test descriptions and organizationThe test descriptions for getter methods could be more specific about what they're testing. For example:
- it('should process getUserId arguments and forwards to getUserId call', () => { + it('should forward getUserId call to analytics instance without modifications', () => {Also consider grouping related tests using describe blocks:
- User-related methods (getUserId, getUserTraits)
- Group-related methods (getGroupId, getGroupTraits)
- Session-related methods (startSession, endSession, getSessionId)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts(4 hunks)packages/analytics-js/src/app/RudderAnalytics.ts(15 hunks)packages/analytics-js/src/components/core/Analytics.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/analytics-js/src/app/RudderAnalytics.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1907
File: packages/analytics-js/src/app/RudderAnalytics.ts:68-68
Timestamp: 2024-11-07T05:29:11.813Z
Learning: In `packages/analytics-js/src/app/RudderAnalytics.ts`, the constructor of the `RudderAnalytics` class intentionally returns `RudderAnalytics.globalSingleton` to implement a singleton pattern. In JavaScript, returning an object from a constructor causes the `new` operator to return that object instead of a new instance. Therefore, when reviewing, avoid flagging returns from constructors as issues if they are intended for singleton implementations.
🪛 Biome
packages/analytics-js/src/app/RudderAnalytics.ts
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
🪛 GitHub Check: codecov/patch
packages/analytics-js/src/components/core/Analytics.ts
[warning] 183-183: packages/analytics-js/src/components/core/Analytics.ts#L183
Added line #L183 was not covered by tests
🔇 Additional comments (5)
packages/analytics-js/src/app/RudderAnalytics.ts (2)
61-105: LGTM! Error handling added to constructor.
The try-catch block and error event dispatch mechanism have been correctly implemented. The constructor return statement is intentionally used for the singleton pattern implementation.
🧰 Tools
🪛 Biome
[error] 65-65: The constructor should not return a value.
The constructor is here:
Returning a value from a constructor is ignored.
(lint/correctness/noConstructorReturn)
503-556: LGTM! Consistent error handling in getter methods.
The error handling implementation is consistent across all getter methods with proper:
- Error catching and reporting
- Undefined return values on errors
- Input sanitization
packages/analytics-js/src/components/core/Analytics.ts (1)
183-183: 🛠️ Refactor suggestion
Consider using a more descriptive error message.
The error message "Life cycle failure" is quite generic and may not provide enough context for debugging. Consider including more specific information about which lifecycle phase failed.
Apply this change to improve error context:
- this.errorHandler.onError(getMutatedError(err, 'Life cycle failure'), ANALYTICS_CORE);
+ this.errorHandler.onError(
+ getMutatedError(err, `Lifecycle failure in phase: ${state.lifecycle.status.value}`),
+ ANALYTICS_CORE
+ );The error handling code lacks test coverage. Let's verify if there are any existing tests for the lifecycle error handling:
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 183-183: packages/analytics-js/src/components/core/Analytics.ts#L183
Added line #L183 was not covered by tests
packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (2)
753-853: Well-structured page lifecycle event tracking tests!
The test suite for trackPageLifecycleEvents is comprehensive and well-organized:
- Covers configuration inheritance from autoTrack options
- Tests feature toggle behavior
- Verifies event timing and content
- Includes edge cases like useBeacon interaction
218-239:
Fix inconsistent error messages in ready test case
The error message thrown by getAnalyticsInstance doesn't match the expected error in the assertion:
- Thrown:
'Error in getAnalyticsInstance' - Expected:
"Cannot read properties of undefined (reading 'writeKey')"
Update either the mock implementation or the assertion to ensure consistency:
- throw new Error('Error in getAnalyticsInstance');
+ throw new TypeError("Cannot read properties of undefined (reading 'writeKey')");Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/analytics-js-common/__tests__/utilities/errors.test.ts (1)
1-13: Consider adding integration tests for error handling.Since this is part of implementing error handling for all public APIs, consider adding integration tests that verify:
- Error propagation from public APIs to this error event dispatch
- Error event handling by consumers
Example integration test structure:
// In a separate integration test file describe('Error handling integration', () => { it('should propagate API errors to error events', async () => { const errorHandler = jest.fn(); window.addEventListener('error', errorHandler); // Trigger an error through a public API await analytics.track(undefined as any); expect(errorHandler).toHaveBeenCalled(); const errorEvent = errorHandler.mock.calls[0][0]; expect(errorEvent.error).toBeInstanceOf(Error); window.removeEventListener('error', errorHandler); }); });packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (3)
86-111: Consider refactoring repetitive error handling test cases.The error handling test cases follow a consistent pattern but contain significant code duplication. Consider extracting the common test setup and assertion logic into a helper function.
const testErrorHandling = ( methodName: string, methodCall: () => any, expectedReturnValue?: any ) => { it(`should dispatch an error event if an exception is thrown during the ${methodName} call`, () => { const dispatchEventSpy = jest.spyOn(window, 'dispatchEvent'); const getAnalyticsInstanceSpy = jest .spyOn(rudderAnalytics, 'getAnalyticsInstance') .mockImplementation(() => { throw new Error('Error in getAnalyticsInstance'); }); const result = methodCall(); expect(dispatchEventSpy).toHaveBeenCalledWith( new ErrorEvent('error', { error: new Error('Error in getAnalyticsInstance'), }), ); if (expectedReturnValue !== undefined) { expect(result).toBe(expectedReturnValue); } dispatchEventSpy.mockRestore(); getAnalyticsInstanceSpy.mockRestore(); }); }; // Usage example: testErrorHandling('getUserId', () => rudderAnalytics.getUserId(), undefined);Also applies to: 161-178, 197-213, 222-243, 254-275, 282-302, 310-330, 338-358, 366-386, 394-414, 428-450, 457-478, 505-525, 543-563, 581-606, 609-631, 634-656, 659-681, 684-706, 709-729, 732-754
757-857: LGTM! Comprehensive test coverage for page lifecycle events.The test suite thoroughly covers various configuration scenarios and edge cases for page lifecycle event tracking. Good job on:
- Testing configuration inheritance and overrides
- Simulating page unload events
- Verifying event payload structure
- Proper test isolation with beforeEach/afterEach hooks
Consider adding a test case for the scenario where
autoTrack.pageLifecycleis undefined to ensure proper fallback behavior.
58-77: Enhance test descriptions for better clarity.While the test implementation is good, the descriptions could be more specific about the scenarios being tested. For example:
- it('should return an empty array when globalThis.rudderanalytics is not an array', () => { + it('should return an empty array when globalThis.rudderanalytics is undefined', () => { - it('should return buffered events array when globalThis.rudderanalytics is an array', () => { + it('should return all buffered events when globalThis.rudderanalytics contains valid event calls', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/analytics-js-common/__tests__/utilities/errors.test.ts(1 hunks)packages/analytics-js/__tests__/app/RudderAnalytics.test.ts(4 hunks)
🔇 Additional comments (2)
packages/analytics-js-common/__tests__/utilities/errors.test.ts (2)
1-1: LGTM!
Clean and focused import statement.
3-4: LGTM!
Well-structured test hierarchy with clear naming.
packages/analytics-js-common/__tests__/utilities/errors.test.ts
Outdated
Show resolved
Hide resolved
saikumarrs
force-pushed
the
feat.add-error-handlers-sdk-2256
branch
from
7b8ad4e
to
19ac7a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-common/src/utilities/errors.ts (1)
22-27: Consider adding JSDoc documentation for the error handling strategy.Since this is a central part of the public API error handling strategy, comprehensive documentation would help maintainers and consumers understand:
- The purpose of manual error identification
- How errors are propagated
- How to properly implement error handling in consuming code
Add documentation like:
/** * Dispatches an error event with a manual identifier for tracking. * This is part of the public API error handling strategy where errors * are bubbled up to the ErrorHandler module. * * @param error - The error to be dispatched * @example * try { * // API call * } catch (error) { * dispatchErrorEvent(error); * } */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
.github/workflows/deploy-npm.ymlis excluded by!**/*.yml.github/workflows/deploy-sanity-suite.ymlis excluded by!**/*.yml.github/workflows/deploy.ymlis excluded by!**/*.yml.github/workflows/publish-new-release.ymlis excluded by!**/*.yml
📒 Files selected for processing (4)
CODEOWNERS(1 hunks)packages/analytics-js-common/src/utilities/errors.ts(2 hunks)packages/analytics-js/.size-limit.mjs(2 hunks)packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/.size-limit.mjs
🔇 Additional comments (6)
CODEOWNERS (1)
3-8: LGTM! Consistent ownership transfer for integration package.
The ownership transfer to @rudderlabs/integrations is consistently applied across all related directories including source, tests, fixtures, mocks, and documentation.
packages/analytics-js-common/src/utilities/errors.ts (3)
4-4: LGTM! Clear and descriptive constant declaration.
The constant MANUAL_ERROR_IDENTIFIER is well-named and serves its purpose of marking manually dispatched errors.
29-29: LGTM! Clean exports statement.
All necessary utilities are properly exported.
26-26:
Verify browser compatibility for IE11.
The code uses globalThis and ErrorEvent which might not be fully supported in IE11. Since IE11 support is mentioned in the PR objectives, this needs verification.
Consider adding polyfills or fallbacks:
const globalScope = typeof globalThis !== 'undefined' ? globalThis :
typeof window !== 'undefined' ? window :
typeof global !== 'undefined' ? global :
typeof self !== 'undefined' ? self : {};✅ Verification successful
Let me check for any polyfills or browser compatibility handling in the codebase.
Let me check the actual implementation of error handling and browser detection to see how IE11 compatibility is managed.
Based on the gathered information, I can now provide the final response:
Browser compatibility for IE11 is properly handled
The codebase already includes IE11 detection and handling:
- Dedicated
isIE11()detection in the capabilities manager - Browser detection is used to adapt functionality accordingly
- The error handling is part of a larger system that considers browser capabilities
The use of globalThis and ErrorEvent in this context is safe because:
- The code is part of the error handling system that's integrated with the capabilities manager
- IE11 detection is performed during initialization, allowing for appropriate fallbacks
- The error handling system is designed to work across different browser environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for globalThis polyfill
rg -l "globalThis" --type ts
# Check for ErrorEvent polyfill or type definitions
rg -l "ErrorEvent" --type ts
Length of output: 3692
Script:
#!/bin/bash
# Check for polyfill implementations or browser compatibility handling
rg -A 5 "globalThis|typeof window" packages/analytics-js-common/src/utilities/errors.ts packages/analytics-js-common/src/utilities/page.ts packages/analytics-js/src/components/utilities/globals.ts
# Check package.json for any polyfill dependencies
cat package.json packages/*/package.json 2>/dev/null | jq -r 'select(.dependencies != null) | .dependencies | keys[]' | grep -i "polyfill"
# Look for IE11 related configurations or comments
rg -i "ie11|internet explorer" --type ts
Length of output: 5864
Script:
#!/bin/bash
# Check error handling implementation
rg -A 10 "dispatchErrorEvent" packages/analytics-js-common/src/utilities/errors.ts
# Check IE11 specific handling
rg -A 10 "isIE11" packages/analytics-js/src/components/capabilitiesManager/CapabilitiesManager.ts
# Look for any error event fallback mechanisms
ast-grep --pattern 'new ErrorEvent($$$)'
Length of output: 8379
packages/analytics-js/src/services/ErrorHandler/ErrorHandler.ts (2)
17-17: LGTM: Import statement is correctly placed
The import of MANUAL_ERROR_IDENTIFIER is properly organized with other utilities from the common package.
172-174: Consider enhancing error categorization and reporting
While the current implementation handles manual errors, consider enhancing the error handling architecture to:
- Implement error categorization (e.g., API errors, validation errors, network errors)
- Add structured error metadata for better debugging
- Consider adding error severity levels for better error reporting granularity
This would align better with the PR's objective of comprehensive API error handling.
Let's verify the current error handling coverage:
|
This reverts commit 9fbaf81. revert: error handling changes to all public apis
This reverts commit ef1a977.
PR Description
Error handling has been added to all the consumer-facing APIs. The handled error is bubbled up to the ErrorHandler module by dispatching an "error" event.
The error will show up as unhandled.
However, now the APIs do not cause any crashes in the consumer applications.
I've refactored and move the data sanitization logic to the event overloads utilities as they'll be indirectly used by the integration SDKs as well.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2256/add-error-handlers-at-top-level-for-public-apis-js-sdk
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
RSAfor analytics logging.dispatchErrorEventfor global error event handling.getSanitizedValue.Improvements
undefinedreturns, improving error handling.RudderAnalyticsclass.ErrorHandlerclass.Bug Fixes
Tests
RudderAnalyticsclass.dispatchErrorEventfunction.