fix: event API overloads #1782
fix: event API overloads #1782
Conversation
WalkthroughWalkthroughThe recent changes enhance the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range, codebase verification and nitpick comments (6)
packages/sanity-suite/src/testBookSuites/page.js (4)
9-12: Add a comment to describe the purpose of the new imports.Adding comments for new imports can improve code readability and maintainability.
// Import expected data for additional page method test cases import page9ExpectedData from '../../__fixtures__/page9.json'; import page10ExpectedData from '../../__fixtures__/page10.json'; import page11ExpectedData from '../../__fixtures__/page11.json'; import page12ExpectedData from '../../__fixtures__/page12.json';
18-28: Update the description to reflect the new overloads.The description should include the new
pagemethod overloads for completeness.'Page Method: \ rudderanalytics.page(category, name, [properties], [options], [callback]);\ rudderanalytics.page(category, name, [properties], [callback]);\ rudderanalytics.page(category, name, [callback]);\ rudderanalytics.page(name, [properties], [options], [callback]);\ rudderanalytics.page(name, [properties], [callback]);\ rudderanalytics.page(name, [callback]);\ rudderanalytics.page(properties, [options], [callback]);\ rudderanalytics.page(properties, [callback]);\ rudderanalytics.page([callback]);\ + rudderanalytics.page(name, properties, options);\ + rudderanalytics.page(name, properties);\ + rudderanalytics.page(properties, options);\ + rudderanalytics.page(properties);\
352-493: Missing Expected Data Files for Test CasesThe expected data files for
page11andpage12are missing. Ensure these files are added to the repository and contain valid JSON to verify the new test cases correctly.
packages/sanity-suite/src/__fixtures__/page11.jsonpackages/sanity-suite/src/__fixtures__/page12.jsonAnalysis chain
Verify the new test cases for correctness.
The new test cases seem to be correctly testing the
pagemethod with various argument combinations. Ensure that the expected results inpage11ExpectedDataandpage12ExpectedDataare accurate and cover all edge cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the expected data files for the new test cases. # Test: Check if the expected data files exist and contain valid JSON. for i in {11..12}; do if [ ! -f "packages/sanity-suite/src/__fixtures__/page${i}.json" ]; then echo "Expected data file page${i}.json is missing." else jq empty "packages/sanity-suite/src/__fixtures__/page${i}.json" || echo "Invalid JSON in page${i}.json" fi doneLength of output: 365
270-350: Expected data files for new test cases are missing.The expected data files (
page9.json,page10.json,page11.json, andpage12.json) required to verify the new test cases are not present in the repository. Please add these files to ensure the test cases can be correctly validated.
packages/sanity-suite/src/__fixtures__/page9.jsonpackages/sanity-suite/src/__fixtures__/page10.jsonpackages/sanity-suite/src/__fixtures__/page11.jsonpackages/sanity-suite/src/__fixtures__/page12.jsonAnalysis chain
Verify the new test cases for correctness.
The new test cases seem to be correctly testing the
pagemethod with various argument combinations. Ensure that the expected results inpage9ExpectedDataandpage10ExpectedDataare accurate and cover all edge cases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the expected data files for the new test cases. # Test: Check if the expected data files exist and contain valid JSON. for i in {9..12}; do if [ ! -f "packages/sanity-suite/src/__fixtures__/page${i}.json" ]; then echo "Expected data file page${i}.json is missing." else jq empty "packages/sanity-suite/src/__fixtures__/page${i}.json" || echo "Invalid JSON in page${i}.json" fi doneLength of output: 724
packages/analytics-js/__tests__/components/core/Analytics.test.ts (2)
449-450: Fix the typo in the test description.There is a typo in the test description. "sent" should be "send".
- it('should sent events if loaded', () => { + it('should send events if loaded', () => {
632-633: Fix the typo in the test description.There is a typo in the test description. "sent" should be "send".
- it('should sent events if loaded', () => { + it('should send events if loaded', () => {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (12)
packages/sanity-suite/__fixtures__/alias5.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/alias6.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/group6.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/group7.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/identify8.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/identify9.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/page10.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/page11.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/page12.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/page9.jsonis excluded by!**/*.jsonpackages/sanity-suite/__fixtures__/track5.jsonis excluded by!**/*.jsonpackages/sanity-suite/package.jsonis excluded by!**/*.json
Files selected for processing (16)
- packages/analytics-js-common/tests/utilities/eventMethodOverloads.test.ts (8 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/types/traits.ts (2 hunks)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (9 hunks)
- packages/analytics-js/tests/app/RudderAnalytics.test.ts (2 hunks)
- packages/analytics-js/tests/components/core/Analytics.test.ts (2 hunks)
- packages/analytics-js/tests/components/eventManager/RudderEventFactory.test.ts (1 hunks)
- packages/analytics-js/src/app/RudderAnalytics.ts (5 hunks)
- packages/analytics-js/src/components/eventManager/RudderEventFactory.ts (1 hunks)
- packages/sanity-suite/.env.example (1 hunks)
- packages/sanity-suite/src/testBookSuites/alias.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/group.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/identify.js (9 hunks)
- packages/sanity-suite/src/testBookSuites/page.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/track.js (6 hunks)
Files skipped from review due to trivial changes (2)
- packages/analytics-js/tests/components/eventManager/RudderEventFactory.test.ts
- packages/sanity-suite/.env.example
Additional context used
Biome
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (81)
packages/analytics-js-common/src/types/ApiObject.ts (1)
13-13: The change is approved.Adding
Dateas a valid type within arrays inApiObjectis consistent with the existing structure and enhances flexibility.packages/analytics-js-common/src/types/traits.ts (1)
88-89: The changes are approved.Adding
Dateas a valid type withinIdentifyTraitsis consistent with the existing structure. Removing theconstructorfield avoids potential conflicts with object construction.packages/analytics-js/src/components/eventManager/RudderEventFactory.ts (6)
Line range hint
11-22:
The method implementation is approved.The
generatePageEventmethod correctly generates a 'page' event and aligns with the updated type definitions.
Line range hint
24-32:
The method implementation is approved.The
generateTrackEventmethod correctly generates a 'track' event and aligns with the updated type definitions.
Line range hint
34-43:
The method implementation is approved.The
generateIdentifyEventmethod correctly generates an 'identify' event and aligns with the updated type definitions.
Line range hint
45-56:
The method implementation is approved.The
generateAliasEventmethod correctly generates an 'alias' event and aligns with the updated type definitions.
Line range hint
58-71:
The method implementation is approved.The
generateGroupEventmethod correctly generates a 'group' event and aligns with the updated type definitions.
Line range hint
73-112:
The method implementation is approved.The
createmethod correctly generates a newRudderEventobject based on the event type and aligns with the updated type definitions.packages/analytics-js-common/src/types/IRudderAnalytics.ts (5)
10-17: The changes are approved.The overloads for
AnalyticsIdentifyMethodare correctly defined and enhance flexibility in method invocation.
23-41: The changes are approved.The overloads for
AnalyticsPageMethodare correctly defined and enhance flexibility in method invocation.
51-52: The changes are approved.The overloads for
AnalyticsTrackMethodare correctly defined and enhance flexibility in method invocation.
57-65: The changes are approved.The overloads for
AnalyticsGroupMethodare correctly defined and enhance flexibility in method invocation.
69-72: The changes are approved.The overloads for
AnalyticsAliasMethodare correctly defined and enhance flexibility in method invocation.packages/sanity-suite/src/testBookSuites/group.js (9)
7-7: LGTM!The import statement for
group7ExpectedDatais consistent with the other import statements.
12-19: LGTM!The descriptions for the
rudderanalytics.groupmethod overloads are clear and cover various argument combinations.
23-23: LGTM!The test case description for
rudderanalytics.group(groupId, traits, options)is clear.
105-105: LGTM!The test case description for
rudderanalytics.group(groupId, traits)is clear.
138-138: LGTM!The test case description for
rudderanalytics.group(groupId, traits, null)is clear.
172-172: LGTM!The test case description for
rudderanalytics.group(groupId, null, options)is clear.
234-234: LGTM!The test case description for
rudderanalytics.group(groupId)is clear.
241-241: LGTM!The test case description for
rudderanalytics.group(traits, options)is clear.
320-320: LGTM!The test case description for
rudderanalytics.group(traits)is clear.packages/analytics-js/__tests__/app/RudderAnalytics.test.ts (2)
132-133: LGTM!The test case for the
rudderanalytics.pagemethod correctly verifies the argument processing and forwarding to thepagecall.
165-166: LGTM!The test case for the
rudderanalytics.groupmethod correctly verifies the argument processing and forwarding to thegroupcall.packages/analytics-js-common/__tests__/utilities/eventMethodOverloads.test.ts (30)
23-28: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
33-33: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
37-37: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
41-48: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
51-52: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
56-63: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
66-70: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
73-76: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
79-81: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
83-87: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
89-90: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
95-98: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
103-106: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
111-116: LGTM!The test case for
pageArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
129-133: LGTM!The test case for
trackArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
135-137: LGTM!The test case for
trackArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
139-141: LGTM!The test case for
trackArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
148-149: LGTM!The test case for
trackArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
176-180: LGTM!The test case for
identifyArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
187-190: LGTM!The test case for
identifyArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
196-198: LGTM!The test case for
identifyArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
208-213: LGTM!The test case for
identifyArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
232-236: LGTM!The test case for
aliasArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
243-245: LGTM!The test case for
aliasArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
253-257: LGTM!The test case for
aliasArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
276-280: LGTM!The test case for
groupArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
287-290: LGTM!The test case for
groupArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
296-297: LGTM!The test case for
groupArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
308-313: LGTM!The test case for
groupArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.
315-319: LGTM!The test case for
groupArgumentsToCallOptionscorrectly verifies the argument processing for the provided arguments.packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (5)
147-151: LGTM!The code changes correctly handle the combination of
nameandcategoryproperties inpageArgumentsToCallOptions.
156-157: LGTM!The code changes correctly add
nameandcategoryto thepropertiesobject inpageArgumentsToCallOptions.
213-213: LGTM!The code change correctly updates the
identifyArgumentsToCallOptionsfunction to acceptstringornumberforuserId.
282-282: LGTM!The code change correctly updates the
aliasArgumentsToCallOptionsfunction to accept different argument combinations.
393-397: LGTM!The code changes correctly handle the
traitsproperty ingroupArgumentsToCallOptions.packages/sanity-suite/src/testBookSuites/alias.js (6)
Line range hint
21-120:
LGTM!The test case
alias1is well-structured and covers a comprehensive range of data points for thealiasmethod.
124-171: LGTM!The test case
alias2is well-structured and covers a comprehensive range of data points for thealiasmethod.
175-222: LGTM!The test case
alias3is well-structured and covers a comprehensive range of data points for thealiasmethod.
Line range hint
226-273:
LGTM!The test case
alias4is well-structured and covers a comprehensive range of data points for thealiasmethod.
276-280: LGTM!The test case
alias5is well-structured and covers a comprehensive range of data points for thealiasmethod.
Line range hint
284-382:
LGTM!The test case
alias6is well-structured and covers a comprehensive range of data points for thealiasmethod.packages/sanity-suite/src/testBookSuites/track.js (6)
Line range hint
20-136:
LGTM!The test case
track1is well-structured and covers a comprehensive range of data points for thetrackmethod.
Line range hint
140-207:
LGTM!The test case
track2is well-structured and covers a comprehensive range of data points for thetrackmethod.
Line range hint
211-279:
LGTM!The test case
track3is well-structured and covers a comprehensive range of data points for thetrackmethod.
Line range hint
283-383:
LGTM!The test case
track4is well-structured and covers a comprehensive range of data points for thetrackmethod.
387-390: LGTM!The test case
track5is well-structured and covers a comprehensive range of data points for thetrackmethod.
Line range hint
394-402:
LGTM!The test case
track6is well-structured and covers a comprehensive range of data points for thetrackmethod.packages/analytics-js/src/app/RudderAnalytics.ts (5)
176-205: LGTM!The
pagemethod overloads are well-structured and comprehensive. The implementation correctly processes the arguments.
221-229: LGTM!The
trackmethod overloads are well-structured and comprehensive. The implementation correctly processes the arguments.
244-258: LGTM!The
identifymethod overloads are well-structured and comprehensive. The implementation correctly processes the arguments.
273-277: LGTM!The
aliasmethod overloads are well-structured and comprehensive. The implementation correctly processes the arguments.
290-304: LGTM!The
groupmethod overloads are well-structured and comprehensive. The implementation correctly processes the arguments.packages/sanity-suite/src/testBookSuites/identify.js (5)
Line range hint
25-122:
LGTM!The test case
identify1is well-structured and covers a comprehensive range of data points for theidentifymethod.
Line range hint
124-170:
LGTM!The test case
identify2is well-structured and covers a comprehensive range of data points for theidentifymethod.
Line range hint
174-234:
LGTM!The test case
identify3is well-structured and covers a comprehensive range of data points for theidentifymethod.
236-241: LGTM!The test case
identify4is well-structured and covers a comprehensive range of data points for theidentifymethod.
Line range hint
243-290:
LGTM!The test case
identify5is well-structured and covers a comprehensive range of data points for theidentifymethod.
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Outdated
Show resolved
Hide resolved
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1782 +/- ##
===========================================
+ Coverage 56.42% 56.44% +0.01%
===========================================
Files 467 467
Lines 15974 15967 -7
Branches 3212 3210 -2
===========================================
- Hits 9014 9013 -1
+ Misses 5690 5684 -6
Partials 1270 1270 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/sanity-suite/package.jsonis excluded by!**/*.json
Files selected for processing (2)
- packages/analytics-js-plugins/src/deviceModeDestinations/types.ts (1 hunks)
- packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2 hunks)
Additional comments not posted (3)
packages/analytics-js-plugins/src/deviceModeDestinations/types.ts (1)
5-18: LGTM! The type definition changes enhance flexibility and robustness.The updates to
DeviceModeDestinationsAnalyticsInstancealign with the PR objectives, leveragingIRudderAnalyticsand additional properties.packages/analytics-js-plugins/src/deviceModeDestinations/utils.ts (2)
18-18: LGTM! The import ofAnonymousIdOptionsaligns with the updated function signatures.The addition of
AnonymousIdOptionsis consistent with the changes in the function signatures.
59-127: LGTM! The function updates align with the new type definitions and method signatures.The changes to
createDestinationInstanceare consistent with the updated type definitions and method signatures.Ensure that all usages of
createDestinationInstanceare consistent with the new function signatures.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (11)
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (11)
109-110: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;Tools
Biome
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
117-118: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;Tools
Biome
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
147-150: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.category; + payload.category = undefined;
148-149: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.name; + payload.name = undefined;
150-151: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.properties; + payload.properties = undefined;
151-152: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;
Line range hint
133-134:
Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;
250-251: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;
319-320: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.to; + payload.to = undefined;
325-326: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.from; + payload.from = undefined;
380-381: Avoid using thedeleteoperator for performance reasons.Using the
deleteoperator can impact performance. Instead, consider setting the value toundefined.- delete payload.options; + payload.options = undefined;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-common/src/types/EventApi.ts (1 hunks)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (10 hunks)
- packages/analytics-js/src/components/eventManager/RudderEventFactory.ts (3 hunks)
Files skipped from review due to trivial changes (1)
- packages/analytics-js-common/src/types/EventApi.ts
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/src/components/eventManager/RudderEventFactory.ts
Additional context used
Learnings (1)
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (1)
Learnt from: saikumarrs PR: rudderlabs/rudder-sdk-js#1782 File: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts:247-251 Timestamp: 2024-07-11T05:26:03.507Z Learning: The `delete` operator can impact performance because it changes the structure of the object and invalidates JavaScript engine optimizations like hidden classes and inline caches.
Biome
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
[error] 109-109: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 117-117: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackage.jsonis excluded by!**/*.json
Files selected for processing (1)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-common/tests/utilities/eventMethodOverloads.test.ts (7 hunks)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (18 hunks)
- packages/analytics-js/tests/components/core/Analytics.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
- packages/analytics-js/tests/components/core/Analytics.test.ts
Additional comments not posted (5)
packages/analytics-js-common/__tests__/utilities/eventMethodOverloads.test.ts (5)
23-23: LGTM: Adding test case forpageArgumentsToCallOptionswith undefined callback.The test case correctly handles the scenario where the callback is not provided.
28-29: LGTM: Adding test case forpageArgumentsToCallOptionswith undefined callback.The test case correctly handles the scenario where the callback is not provided.
34-35: LGTM: Adding test case forpageArgumentsToCallOptionswith undefined options and callback.The test case correctly handles the scenario where both options and callback are not provided.
40-43: LGTM: Adding test case forpageArgumentsToCallOptionswith undefined options and callback.The test case correctly handles the scenario where both options and callback are not provided.
47-54: LGTM: Adding test case forpageArgumentsToCallOptionswith provided callback and undefined options.The test case correctly handles the scenario where the callback is provided, but options are not.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (16 hunks)
- packages/sanity-suite/src/testBookSuites/alias.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/group.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/identify.js (9 hunks)
- packages/sanity-suite/src/testBookSuites/page.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/track.js (6 hunks)
Files skipped from review as they are similar to previous changes (6)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
- packages/sanity-suite/src/testBookSuites/alias.js
- packages/sanity-suite/src/testBookSuites/group.js
- packages/sanity-suite/src/testBookSuites/identify.js
- packages/sanity-suite/src/testBookSuites/page.js
- packages/sanity-suite/src/testBookSuites/track.js
Co-authored-by: Moumita <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/sanity-suite/src/testBookSuites/alias.js (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sanity-suite/src/testBookSuites/alias.js
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
packages/analytics-js-service-worker/package.jsonis excluded by!**/*.json
Files selected for processing (1)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/analytics-js-integrations/rollup.config.mjs (3 hunks)
- packages/analytics-v1.1/rollup-configs/rollup.utilities.mjs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/analytics-js-integrations/rollup.config.mjs
- packages/analytics-v1.1/rollup-configs/rollup.utilities.mjs
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (16 hunks)
- packages/sanity-suite/src/testBookSuites/alias.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/group.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/identify.js (9 hunks)
- packages/sanity-suite/src/testBookSuites/page.js (5 hunks)
- packages/sanity-suite/src/testBookSuites/track.js (6 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/sanity-suite/src/testBookSuites/group.js
- packages/sanity-suite/src/testBookSuites/identify.js
- packages/sanity-suite/src/testBookSuites/page.js
- packages/sanity-suite/src/testBookSuites/track.js
Additional context used
Learnings (1)
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (3)
Learnt from: saikumarrs PR: rudderlabs/rudder-sdk-js#1782 File: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts:0-0 Timestamp: 2024-07-11T08:44:37.825Z Learning: In `packages/analytics-js-common/src/utilities/eventMethodOverloads.ts`, the `delete` operator has been replaced with setting the value to `undefined` for better performance.Learnt from: saikumarrs PR: rudderlabs/rudder-sdk-js#1782 File: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts:0-0 Timestamp: 2024-07-11T08:44:26.185Z Learning: The `delete` operator in `packages/analytics-js-common/src/utilities/eventMethodOverloads.ts` should be replaced with setting the value to `undefined` to avoid performance issues.Learnt from: saikumarrs PR: rudderlabs/rudder-sdk-js#1782 File: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts:0-0 Timestamp: 2024-07-11T08:44:39.648Z Learning: Avoid using the `delete` operator for performance reasons. Instead, set the value to `undefined` in the `packages/analytics-js-common/src/utilities/eventMethodOverloads.ts` file.
GitHub Check: codecov/patch
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
[warning] 263-263: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts#L263
Added line #L263 was not covered by tests
[warning] 393-393: packages/analytics-js-common/src/utilities/eventMethodOverloads.ts#L393
Added line #L393 was not covered by tests
Additional comments not posted (8)
packages/sanity-suite/src/testBookSuites/alias.js (7)
11-16: Updated method descriptions are comprehensive and accurate.The updated descriptions now correctly reflect the various ways the
aliasmethod can be invoked, aligning with the PR's goal to clearly document API usages.
20-22: Test Case 'alias1': Comprehensive input and trigger setup.The test case for
alias(to, from, apiOptions)is well-prepared with detailed input data and a clear trigger handler setup. This ensures thorough testing of the method's functionality with complex input structures.Also applies to: 119-119
123-124: Test Case 'alias2': Clear and concise setup.The test case for
alias(to, from)is straightforward and effectively tests the basic functionality of the method.Also applies to: 170-170
174-175: Test Case 'alias3': Handles edge cases with null values.The test case specifically checks the handling of
nullvalues in the third parameter, which is a good practice to ensure robustness.Also applies to: 221-221
225-226: Test Case 'alias4': Tests method with prior identification.This test case ensures that the
aliasmethod behaves correctly when a user has been previously identified, which is crucial for maintaining user continuity.Also applies to: 272-272
275-276: Test Case 'alias5': Verifies behavior without prior identification.It's important to test the
aliasmethod in scenarios where no prior identification has occurred, and this test case does just that.Also applies to: 279-279
283-285: Test Case 'alias6': Detailed testing of API options handling.The test case is designed to check how API options are handled, and it includes a comprehensive set of inputs to test various scenarios.
Also applies to: 381-381
packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (1)
33-34: Alias method updates are well-handled.The changes to the
aliasArgumentsToCallOptionsmethod correctly handle various input types and ensure that undefined values are managed properly. The updates improve the method's robustness and flexibility.Also applies to: 304-304, 329-329
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts (14 hunks)
- packages/analytics-js-plugins/tests/deviceModeDestinations/utils.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-common/src/utilities/eventMethodOverloads.ts
Additional context used
Learnings (1)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (1)
Learnt from: saikumarrs PR: rudderlabs/rudder-sdk-js#1708 File: packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts:10-11 Timestamp: 2024-05-06T08:40:17.868Z Learning: The misuse of `IHttpClient` in type assertions within the file `packages/analytics-js-plugins/__tests__/deviceModeTransformation/index.test.ts` has been corrected by the user.
Additional comments not posted (6)
packages/analytics-js-plugins/__tests__/deviceModeDestinations/utils.test.ts (6)
116-116: MethodgetAnonymousIdlooks good.This method correctly returns a hardcoded 'anonymousId', which is suitable for testing purposes.
117-117: MethodgetUserIdis correctly implemented.This method properly returns a hardcoded 'userId', which is intended for use in testing scenarios.
118-118: MethodgetUserTraitsis implemented correctly.It returns a predefined set of user traits, which is appropriate for testing the functionality of trait handling.
119-119: MethodgetGroupIdis implemented correctly.This method properly returns a hardcoded 'groupId', suitable for testing the group ID retrieval functionality.
120-120: MethodgetGroupTraitsis implemented correctly.It returns a predefined set of group traits, which is appropriate for testing the functionality of group trait handling.
121-121: MethodgetSessionIdis implemented correctly.This method properly returns a hardcoded 'sessionId', suitable for testing the session ID retrieval functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (4)
nx.jsonis excluded by!**/*.jsonpackage.jsonis excluded by!**/*.jsonpackages/analytics-js-service-worker/package.jsonis excluded by!**/*.jsonpackages/analytics-v1.1/package.jsonis excluded by!**/*.json
Files selected for processing (2)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml (2 hunks)
- .github/workflows/unit-tests-and-lint.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml
Additional comments not posted (2)
.github/workflows/unit-tests-and-lint.yml (2)
38-39: Added environment variableBASE_REFin unit tests step is correctly implemented.The use of
${{ github.event.pull_request.base.ref || 'HEAD' }}ensures that the tests are run against the correct base branch or default to 'HEAD' if not available. This is a good practice to ensure consistency in testing environments.Please verify this change by running the workflow in different scenarios, such as direct commits to branches and pull requests from forks.
Verification successful
Verified the addition of the
BASE_REFenvironment variable in the workflow file.The
BASE_REFvariable is correctly set using${{ github.event.pull_request.base.ref || 'HEAD' }}in both the unit tests and linting steps. This ensures that the correct base reference is used during these processes, promoting consistency in the testing environment.
- Lines 38-39:
BASE_REFis set before runningnpm run test:ci.- Lines 44-45:
BASE_REFis set before runningnpm run check:lint:ci.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the BASE_REF environment variable in different scenarios # Test: Run the workflow from a direct commit and from a pull request. echo "Check the workflow runs from a direct commit and a pull request to ensure BASE_REF is set correctly."Length of output: 210
Script:
#!/bin/bash # Search for the usage of BASE_REF in the GitHub Actions workflow file rg 'BASE_REF' .github/workflows/unit-tests-and-lint.yml -A 5Length of output: 457
51-52: Added environment variableBASE_REFin linting step is correctly implemented.The consistent use of
${{ github.event.pull_request.base.ref || 'HEAD' }}across different steps in the workflow is a good practice. It ensures that lint checks are also aligned with the base branch of the pull request.Please verify this change by running the workflow in scenarios where the pull request base might change.
Verification successful
Added environment variable
BASE_REFin linting step is correctly implemented.The consistent use of
${{ github.event.pull_request.base.ref || 'HEAD' }}across different steps in the workflow is a good practice. It ensures that lint checks are also aligned with the base branch of the pull request.
.github/workflows/unit-tests-and-lint.yml:BASE_REFis set correctly in both the test and linting steps.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the BASE_REF environment variable in scenarios where the pull request base changes # Test: Modify the base of a pull request and check if BASE_REF updates correctly. echo "Modify the base of a pull request and ensure BASE_REF updates correctly in the linting step."Length of output: 194
Script:
#!/bin/bash # Search for the BASE_REF environment variable definition and its usage in the .github/workflows/unit-tests-and-lint.yml file # Check the definition of BASE_REF echo "Searching for BASE_REF definition in .github/workflows/unit-tests-and-lint.yml" rg 'BASE_REF' .github/workflows/unit-tests-and-lint.yml -A 3Length of output: 464
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
package.jsonis excluded by!**/*.json
Files selected for processing (2)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml (3 hunks)
- .github/workflows/unit-tests-and-lint.yml (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/security-code-quality-and-bundle-size-checks.yml
- .github/workflows/unit-tests-and-lint.yml
|
PR Description
I've addressed all the outstanding issues with event API overloads.
Now, the types declaration files will properly show all the possible ways to invoke the APIs.
madgedependency (to check circular references) has been removed to avoid vulnerability.alias(callback)alias(options, [callback])group(callback)pageAPI that would overwritenameandcategoryfields if present in thepropertiesargument forpage(properties)usage.resetAPI is now called first before the actual API calls.Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2115/fix-issues-in-event-api-overloads-js-sdk
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
Bug Fixes
New Features
alias,group,identify,page,track) to support additional argument combinations, expanding their versatility.getAnonymousId,getUserId, etc.) to retrieve specific analytical data points.Tests
Chores