chore: migrate all lodash usages with ramda in web device mode integrations #1684
Conversation
mihir-4116
requested review from
ItsSudip,
shrouti1507,
sandeepdsvs and
koladilip
as code owners
WalkthroughThe changes primarily focus on refactoring and enhancing the utility functions within the Adobe Analytics integration of the analytics-js library. Key modifications include replacing 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 Configration File (
|
mihir-4116
changed the title
mihir-4116
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/Braze/browser.js (2 hunks)
- packages/analytics-js-integrations/src/integrations/FacebookPixel/utils.js (1 hunks)
- packages/analytics-js-integrations/src/utils/commonUtils.js (3 hunks)
- packages/analytics-js-integrations/src/utils/utils.js (2 hunks)
Additional comments not posted (7)
packages/analytics-js-integrations/src/utils/commonUtils.js (2)
10-13: Ensure theisDefinedfunction from@rudderstack/analytics-js-common/utilities/checksis correctly implemented and exported, as the localisDefinedfunction seems to have been replaced but its implementation is not visible here.
10-13: ThepickByfunction from Ramda is used correctly to replace lodash'spickByfunctionality. However, ensure that the predicate functions (isDefined,isNotNull, etc.) are correctly defined and imported, as their implementations are not visible in this snippet.packages/analytics-js-integrations/src/integrations/FacebookPixel/utils.js (1)
8-8: Ensure theisDefinedfunction from@rudderstack/analytics-js-common/utilities/checksis correctly imported and used throughout the file. This change aligns with the migration goal and helps in maintaining consistency across the codebase.packages/analytics-js-integrations/src/integrations/Braze/browser.js (2)
2-2: The replacement oflodash.isequalwithramda.equalsis correctly implemented. This change aligns with the migration goal and leverages Ramda's functional programming capabilities for value comparison.
201-206: Ensure that theequalsfunction from Ramda is used correctly for deep comparison of objects and dates. This change improves consistency and aligns with the migration from lodash to Ramda.packages/analytics-js-integrations/src/utils/utils.js (1)
4-4: The import ofisDefinedfrom@rudderstack/analytics-js-common/utilities/checksis correctly implemented. This change aligns with the goal of using centralized utility functions and improves code maintainability.packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (1)
5-5: Ensure that all instances previously usingisDefinedfrom../../utils/commonUtilsare now correctly using the new import from@rudderstack/analytics-js-common/utilities/checks.Verification successful
The verification process did not find any remaining references to
isDefinedbeing imported from the old path (../../utils/commonUtils). This suggests that the transition to the new import path from@rudderstack/analytics-js-common/utilities/checkshas been successfully completed.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to the old `isDefined` import path. rg --type js "from '../../utils/commonUtils' import .*isDefined"Length of output: 72
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1684 +/- ##
===========================================
+ Coverage 55.10% 55.47% +0.37%
===========================================
Files 464 464
Lines 15845 15849 +4
Branches 3166 3189 +23
===========================================
+ Hits 8731 8793 +62
+ Misses 5854 5731 -123
- Partials 1260 1325 +65 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
|
@ItsSudip , have we tested all the impacted destinations? |
Not yet. As I have updated some utility functions, it will affect many integrations. Let me test the popular one at least and the directly impacted destinations like Braze, and Adobe Analytics. |
|
Hello! This PR has been open for 20 days without any activity. Therefore, it's considered as stale and is scheduled to be closed in 10 days. If you're still working on this, please remove the 'Stale' label or add a comment to keep it open. Thanks for your contribution! |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-integrations/tests/utils/commonUtils.test.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/Braze/browser.js (5 hunks)
- packages/analytics-js-integrations/src/utils/commonUtils.js (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-integrations/src/utils/commonUtils.js
Additional comments not posted (1)
packages/analytics-js-integrations/src/integrations/Braze/browser.js (1)
Line range hint
2-11: Changes in theidentifymethod and the use of Ramda functions look good.The migration from lodash to Ramda for functions like
equalsandisNotEmptyis correctly implemented. The deduplication logic in theidentifymethod is complex but well-handled, ensuring that only changed user attributes are updated.Also applies to: 190-240
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (1)
Line range hint
132-132: Change to an optional chain to simplify the code and improve readability.- if (properties[rudderProp.eventMerchProperties]) + if (properties?.[rudderProp.eventMerchProperties])
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/analytics-js-integrations/tests/integrations/AdobeAnalytics/util.test.js (1 hunks)
- packages/analytics-js-integrations/tests/utils/commonUtils.test.js (1 hunks)
- packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (2 hunks)
Additional context used
Learnings (1)
packages/analytics-js-integrations/__tests__/utils/commonUtils.test.js (1)
User: ItsSudip PR: rudderlabs/rudder-sdk-js#1684 File: packages/analytics-js-integrations/__tests__/utils/commonUtils.test.js:355-357 Timestamp: 2024-06-10T07:43:52.149Z Learning: The `isBlank` function in `commonUtils.test.js` is expected to return `false` when the input is `null`, as per the specific implementation and requirement of the project.
Biome
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
[error] 132-132: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (5)
packages/analytics-js-integrations/__tests__/integrations/AdobeAnalytics/util.test.js (4)
1-7: The imports and exports are correctly updated to reflect the new utility functions.
9-16: Ensure that thewindowSpymock is correctly restored after each test to prevent side effects in subsequent tests.
18-72: The tests forgetDataFromContextfunction are comprehensive and cover various scenarios. Good job on ensuring edge cases are tested.
180-240: The tests for thegetfunction are thorough and effectively test various edge cases. Ensure that the function's behavior is consistent across all browsers, especially with unusual property names.packages/analytics-js-integrations/__tests__/utils/commonUtils.test.js (1)
349-379: The tests for theisBlankfunction are correctly implemented according to the project's requirements. No changes are needed here as the behavior of treatingnullas not blank is intentional.
packages/analytics-js-integrations/__tests__/integrations/AdobeAnalytics/util.test.js
Show resolved
Hide resolved
packages/analytics-js-integrations/__tests__/integrations/AdobeAnalytics/util.test.js
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
Outdated
Show resolved
Hide resolved
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (1)
Line range hint
132-132: Consider using optional chaining to simplify the code and improve readability.- const { bitrate, startupTime, fps, droppedFrames } = properties; + const { bitrate, startupTime, fps, droppedFrames } = properties || {};This change ensures that the code does not throw an error if
propertiesis undefined, enhancing robustness.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.json,!**/*.jsonpackages/analytics-js-integrations/package.jsonis excluded by!**/*.json
Files selected for processing (1)
- packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js (2 hunks)
Additional context used
Biome
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
[error] 132-132: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
packages/analytics-js-integrations/src/integrations/AdobeAnalytics/util.js
Show resolved
Hide resolved
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/analytics-js-integrations/tests/integrations/AdobeAnalytics/util.test.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-integrations/tests/integrations/AdobeAnalytics/util.test.js
|
PR Description
Linear task (optional)
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
getfunction to enhance performance and accuracy.Tests