fix: type issues #1663
fix: type issues #1663
Conversation
WalkthroughThe updates across various packages primarily focus on enhancing flexibility and simplification in the analytics framework. Key adjustments include reorganizing imports and simplifying type exports in 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- packages/analytics-js-common/src/types/Consent.ts (1 hunks)
- packages/analytics-js/src/components/preloadBuffer/types.ts (1 hunks)
- packages/analytics-js/src/components/utilities/consent.ts (1 hunks)
- packages/analytics-js/src/index.ts (2 hunks)
- packages/analytics-js/src/types/rudderanalytics.d.ts (1 hunks)
- packages/loading-scripts/src/types/rudderanalytics.d.ts (1 hunks)
- packages/sanity-suite/src/types/rudderanalytics.d.ts (1 hunks)
Additional comments: 7
packages/analytics-js/src/components/preloadBuffer/types.ts (1)
- 4-4: The update to
RudderAnalyticsPreloaderto accept any type of arguments (...args: any[]) increases the SDK's flexibility. However, consider using more specific types or generics to maintain type safety where possible.packages/analytics-js/src/types/rudderanalytics.d.ts (1)
- 10-10: The update to the
rudderanalyticsproperty in the globalWindowinterface to supportRudderAnalytics,RudderAnalyticsPreloader, orundefinedenhances the SDK's robustness and flexibility. This is a positive change.packages/loading-scripts/src/types/rudderanalytics.d.ts (1)
- 10-10: The consistent update across packages to allow the
rudderanalyticsproperty in theWindowinterface to beRudderAnalytics,RudderAnalyticsPreloader, orundefinedis commendable. This enhances the SDK's flexibility and robustness.packages/sanity-suite/src/types/rudderanalytics.d.ts (1)
- 10-10: The update to the
rudderanalyticsproperty in the globalWindowinterface across different packages, including the sanity-suite, ensures consistency and enhances the SDK's usability. Good job on maintaining coherent type definitions.packages/analytics-js-common/src/types/Consent.ts (1)
- 34-35: Making the
enabledandproviderfields optional in theConsentManagementOptionstype enhances flexibility in consent management configurations. This is a positive change that improves the SDK's usability.packages/analytics-js/src/index.ts (1)
- 2-2: The changes in this file, including the reorganization of imports, removal of
PreloadedEventCall, and update to therudderanalyticsglobal interface, contribute to a cleaner and more intuitive SDK interface. These modifications align well with the PR's objectives.Also applies to: 25-25, 29-29
packages/analytics-js/src/components/utilities/consent.ts (1)
- 101-102: The updates to the
getConsentManagerInfofunction, including handling theproviderproperty as potentiallyundefinedand the conditional assignment ofconsentManagerPluginName, improve the robustness and flexibility of the consent management logic. These changes are well-aligned with the PR's objectives.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1663 +/- ##
========================================
Coverage 53.89% 53.89%
========================================
Files 461 461
Lines 15588 15588
Branches 3105 3096 -9
========================================
Hits 8401 8401
- Misses 5833 5892 +59
+ Partials 1354 1295 -59 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- packages/analytics-js/src/index.ts (2 hunks)
- packages/loading-scripts/src/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js/src/index.ts
Additional comments: 4
packages/loading-scripts/src/index.ts (4)
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-42]
The method assignment within the loop uses an immediately invoked function expression (IIFE) to create a closure around the
methodName. This is a valid approach to ensure that each method has its own scope. However, consider using ES6 arrow functions for a cleaner syntax and better readability.- (window.rudderanalytics as unknown as RudderAnalyticsPreloader)[method] = (methodName => - function () { - (window.rudderanalytics as unknown as PreloadedEventCall[]).push( - [methodName].concat(Array.prototype.slice.call(arguments) as PreloadedEventCall), - ); - })(method); + (window.rudderanalytics as unknown as RudderAnalyticsPreloader)[method] = ((methodName) => + (...args) => { + (window.rudderanalytics as unknown as PreloadedEventCall[]).push([methodName, ...args]); + } + )(method);
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-42]
The use of
Array.prototype.slice.call(arguments)to convertargumentsto an array is correct but can be simplified with the spread syntax in the context of an arrow function, as suggested in the previous comment.
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-42]
The dynamic import feature detection using
new Function('return import("")')is a clever way to determine if the environment supports modern JavaScript features. However, ensure that this feature detection is necessary for the SDK's target environments and that it aligns with the overall strategy for supporting different JavaScript versions.
- 18-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [21-42]
The polyfill loading logic for
PromiseandglobalThisis important for ensuring compatibility with older browsers. However, consider if there's a more efficient way to include these polyfills, such as through a build step that automatically includes necessary polyfills based on the target environments. This could potentially reduce the need for runtime checks and improve the loading performance.
|
size-limit report 📦
|
PR Description
RudderAnalyticsPreloadertypewindow.rudderanalyticstype to supportundefinedand removedPreloadedEventCall[].Linear task (optional)
https://linear.app/rudderstack/issue/SDK-1400/fix-gcm-typescript-support-issues
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
enabledandproviderfields optional.Refactor
rudderanalyticsproperty to supportRudderAnalytics,RudderAnalyticsPreloader, orundefined, ensuring consistency across the platform.window.rudderanalyticsto ensure type safety in method assignments and event call handling.