fix: sdk loading snippet to reduce ambiguity in updating url #2048
Conversation
📝 WalkthroughWalkthroughThis pull request updates several components of the RudderStack SDK. The Rollup configuration modifies the terser plugin settings by removing the Changes
Possibly Related PRs
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 #2048 +/- ##
========================================
Coverage 61.36% 61.36%
========================================
Files 478 478
Lines 16402 16402
Branches 3228 3224 -4
========================================
Hits 10065 10065
+ Misses 5149 5131 -18
- Partials 1188 1206 +18 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/loading-scripts/src/index.ts (1)
24-26: Consider using environment variables for better maintainability.While the separation of URL components improves clarity, hardcoding values like
sdkVersionandsdkBaseUrlmight make future updates more challenging. Consider using environment variables similar to how__WRITE_KEY__is handled.- const sdkBaseUrl = 'https://cdn.rudderlabs.com'; - const sdkVersion = 'v3'; - const sdkFileName = 'rsa.min.js'; + const sdkBaseUrl = '__SDK_BASE_URL__'; + const sdkVersion = '__SDK_VERSION__'; + const sdkFileName = '__SDK_FILE_NAME__';Then update the
replaceplugin configuration inrollup.config.mjs:replace({ preventAssignment: true, + __SDK_BASE_URL__: process.env.SDK_BASE_URL || 'https://cdn.rudderlabs.com', + __SDK_VERSION__: process.env.SDK_VERSION || 'v3', + __SDK_FILE_NAME__: process.env.SDK_FILE_NAME || 'rsa.min.js', // ... other replacements }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/loading-scripts/rollup.config.mjs(2 hunks)packages/loading-scripts/src/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (2)
packages/loading-scripts/rollup.config.mjs (1)
80-83: LGTM! The terser configuration correctly preserves the new variables.The addition of
sdkVersionandsdkFileNameto bothtop_retainandreservedarrays ensures these variables remain intact during minification, which is essential for the SDK loading snippet to work correctly.Also applies to: 95-98
packages/loading-scripts/src/index.ts (1)
129-133: LGTM! The URL construction is now more maintainable.The new URL construction pattern clearly separates the base URL, version, build type, and file name, making it easier for customers to update individual components, especially when using custom domains.
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
examples/angular/sample-app/src/index.html (1)
12-12: Consider externalizing the CDN URL.While the introduction of separate variables improves clarity, consider externalizing the hardcoded CDN URL (
https://cdn.rudderlabs.com) to a configuration file for better maintainability.- var sdkBaseUrl = "https://cdn.rudderlabs.com"; + var sdkBaseUrl = window.RUDDERSTACK_CDN_URL || "https://cdn.rudderlabs.com";Also applies to: 23-27
examples/integrations/Ninetailed/sample-apps/app-using-v3-cdn/public/index.html (1)
36-38: Minification and Error Handling Note.
In this minified snippet, the error check still usesvoid 0(e.g.,null===(n=window[e][r])||void 0===n||n.apply(...)). This is common in minified outputs for size optimization. If intentional, no action is needed; otherwise, consider verifying that the unminified counterparts use the more readableundefinedfor consistency.packages/sanity-suite/public/v3/manualLoadCall/index-local.html (1)
178-188: Ensure Manual Load Form Functionality
The manual load functionality via the form submission is intact. The script correctly captures form inputs and callswindow.manualLoadwith parsed JSON load options. Verify that users are provided clear error messages if the JSON parsing fails. Consider adding client-side validation for better UX.packages/sanity-suite/public/v3/manualLoadCall/index-cdn.html (1)
47-49: Introduction of New Variables for SDK URL Construction
New variables are introduced:
•sdkBaseUrlnow holds"https://cdn.rudderlabs.com",
•sdkVersionis set to"__CDN_VERSION_PATH__", and
•sdkFileNameis set to"rsa.min.js".This clearly decouples the URL components. Please confirm that using the placeholder
"__CDN_VERSION_PATH__"is intentional (likely to be replaced during your build/deployment process).examples/v3-legacy/index.html (1)
41-44: Commented-Out Modern Build Logic
There is a try-catch block (testing modern build capability via a dynamicFunction) that’s currently commented out. Please confirm whether this is intentional; if modern build support is not required for legacy examples, this is acceptable, but if there’s a plan to re-enable it, consider adding a TODO comment for clarity.examples/v3-beacon/index.html (1)
86-86: Refine SDK Script URL Construction
The URL is now composed using the new variables, which aligns with decoupling the version from the base URL. For improved readability, consider using template literals. For example:- window.rudderAnalyticsAddScript("".concat(sdkBaseUrl, "/").concat(sdkVersion, "/").concat(window.rudderAnalyticsBuildType, "/").concat(sdkFileName), "data-rsa-write-key", "__WRITE_KEY__"); + window.rudderAnalyticsAddScript(`${sdkBaseUrl}/${sdkVersion}/${window.rudderAnalyticsBuildType}/${sdkFileName}`, "data-rsa-write-key", "__WRITE_KEY__");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
examples/angular/sample-app/src/index.html(3 hunks)examples/integrations/Ninetailed/sample-apps/app-using-v3-cdn/public/index.html(1 hunks)examples/nextjs/hooks/sample-app/src/app/layout.tsx(3 hunks)examples/nextjs/js/sample-app/src/app/layout.js(3 hunks)examples/nextjs/page-router/sample-app/src/pages/_document.tsx(3 hunks)examples/nextjs/ts/sample-app/src/app/layout.tsx(3 hunks)examples/reactjs/hooks/sample-app/public/index.html(3 hunks)examples/reactjs/js/sample-app/public/index.html(3 hunks)examples/reactjs/ts/sample-app/public/index.html(3 hunks)examples/reactjs/vite/sample-app/index.html(3 hunks)examples/v3-beacon/index.html(4 hunks)examples/v3-legacy-minimum-plugins/index.html(4 hunks)examples/v3-legacy/index.html(4 hunks)examples/v3-minimum-plugins/index.html(4 hunks)examples/v3/index.html(4 hunks)packages/analytics-js/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js/__tests__/nativeSdkLoader.js(4 hunks)packages/analytics-js/public/index.html(5 hunks)packages/sanity-suite/public/v3/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/index-local.html(4 hunks)packages/sanity-suite/public/v3/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/index-npm.html(3 hunks)packages/sanity-suite/public/v3/integrations/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/integrations/index-local.html(4 hunks)packages/sanity-suite/public/v3/integrations/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/integrations/index-npm.html(3 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-local.html(4 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-npm.html(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
packages/analytics-js/__fixtures__/msw.handlers.ts (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1867
File: packages/analytics-js/__fixtures__/msw.handlers.ts:86-86
Timestamp: 2024-11-12T15:14:33.334Z
Learning: When updating test files to replace 'example.com' with 'dummy.dataplane.host.com', note that the change is only needed for test suites that actually make network requests.
packages/analytics-js/__tests__/nativeSdkLoader.js (1)
Learnt from: saikumarrs
PR: rudderlabs/rudder-sdk-js#1708
File: packages/analytics-js/__tests__/nativeSdkLoader.js:31-33
Timestamp: 2024-11-12T15:14:23.319Z
Learning: The loading snippet in `packages/analytics-js/__tests__/nativeSdkLoader.js` is a standard part of the SDK, and no changes are desired on it.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Security and code quality checks
- GitHub Check: Unit Tests and Lint
🔇 Additional comments (88)
examples/nextjs/page-router/sample-app/src/pages/_document.tsx (2)
12-12: LGTM! Improved URL construction and configuration.The separation of URL components into distinct variables (
sdkBaseUrl,sdkVersion,sdkFileName) makes it easier for customers to update the CDN URL while keeping the version and file name intact. The addition ofscriptLoadingModeexplicitly indicates async loading.Also applies to: 23-27
37-37: Improved readability in error handling.The change from
void 0toundefinedenhances code readability by using the more explicit keyword.examples/nextjs/js/sample-app/src/app/layout.js (1)
20-46: Changes match the previously reviewed file.The modifications are identical to those in
_document.tsxand serve the same purpose.examples/nextjs/hooks/sample-app/src/app/layout.tsx (1)
21-46: Changes match the previously reviewed file.The modifications are identical to those in
_document.tsxand serve the same purpose.examples/nextjs/ts/sample-app/src/app/layout.tsx (2)
21-46: Changes match the previously reviewed file.The modifications are identical to those in
_document.tsxand serve the same purpose.
1-63: Verify cross-browser compatibility.While the changes are primarily structural and don't introduce browser-specific features, please ensure testing across Chrome, Firefox, and IE11 as mentioned in the PR objectives.
packages/analytics-js/__fixtures__/msw.handlers.ts (1)
114-114: LGTM! URL path updated to include explicit version.The change to include "/v3/modern/" in the URL path aligns with the new URL structure and improves clarity.
packages/analytics-js/__tests__/nativeSdkLoader.js (4)
4-4: LGTM! Version bump to 3.0.59.Version update is consistent across all files.
17-18: LGTM! Introduced clear variable names for URL components.The introduction of
sdkVersionandsdkFileNamevariables improves clarity and makes URL updates easier for customers using custom domains.
29-29: LGTM! More explicit undefined check.Changed from
void 0toundefinedfor better readability.
79-79: LGTM! Improved URL construction.The new URL construction using template literals and separate variables makes it clearer and easier to maintain.
examples/reactjs/vite/sample-app/index.html (1)
11-11: Same feedback as Angular sample app.The hardcoded CDN URL should be externalized for better maintainability.
Also applies to: 22-26
examples/reactjs/js/sample-app/public/index.html (3)
31-31: Update RudderSnippetVersion to 3.0.59.
The snippet version has been correctly incremented from "3.0.58" to "3.0.59", which aligns with the intended release update.
42-46: Introduce New SDK Configuration Variables.
New variables—window.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingMode—are added to clearly decouple and define the CDN URL, version, and file name. This makes it easier for customers to update only the base URL if needed.
55-57: Clarify Error Handling Check.
The error handling on method invocation now explicitly uses=== undefined(instead ofvoid 0), which improves readability. Ensure that similar updates are applied consistently throughout the codebase.examples/reactjs/ts/sample-app/public/index.html (3)
31-31: Update RudderSnippetVersion to 3.0.59.
The updated snippet version reflects the latest release. This change is consistent with the overall SDK update.
42-46: Add New SDK URL Components.
AddingsdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModeprovides a modular approach to constructing the SDK URL. This should help decouple versioning from the URL, especially for custom domain setups.
55-57: Enhance Error Handling Readability.
The updated check using(_methodName = window[identifier][methodName]) === null || _methodName === undefined || _methodName.apply(...)is more explicit. Ensure that this pattern is uniformly used in all similar snippets.examples/reactjs/hooks/sample-app/public/index.html (3)
31-31: Update RudderSnippetVersion to 3.0.59.
The change in the snippet version is applied correctly, which keeps this sample app up-to-date with the latest SDK version.
42-46: Configure SDK URL Components.
The introduction ofsdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModestreamlines the configuration process. This modular approach aids in maintainability and easier customization for users.
55-57: Improve Method Invocation Error Handling.
The simplified error handling check (using=== undefinedinstead ofvoid 0) increases clarity. Please verify that similar changes are implemented in all snippets across frameworks.examples/integrations/Ninetailed/sample-apps/app-using-v3-cdn/public/index.html (3)
29-29: Update RudderSnippetVersion in Minified Snippet.
The snippet now shows"3.0.59"immediately upon invocation. Although this file is minified, the version update is evident and correct.
33-34: Set New SDK Configuration Variables in Minified Code.
Within the minified snippet, the introduction ofwindow.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModeis clear. This enables flexible script URL construction.
39-40: Dynamic Build Type Adjustment via Compatibility Check.
The try-catch block usingnew Function(...)updateswindow.rudderAnalyticsBuildTypeto"modern"if the test passes. Double-check that this dynamic adjustment meets all target browser requirements (including IE11) and does not disrupt legacy behavior.packages/sanity-suite/public/v3/index-npm.html (3)
35-35: Update RudderSnippetVersion to 3.0.59.
The snippet’s version is correctly updated to "3.0.59", reflecting the latest SDK release.
46-50: Introduce New SDK URL Parameters.
The addition ofwindow.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModeenhances clarity and modularity in loading the SDK. This change simplifies updates for custom domain users.
59-61: Simplify Undefined Check in Method Invocation.
The error handling statement now explicitly checks forundefinedrather than usingvoid 0, which improves readability. Ensure that this update is applied consistently across all similar scripts.packages/sanity-suite/public/v3/index-npm-bundled.html (3)
35-35: Version Update:
The update ofwindow.RudderSnippetVersionto"3.0.59"correctly reflects the new release version as outlined in the PR objectives.
46-50: Introduction of New SDK Configuration Variables:
The addition ofwindow.rudderAnalyticsBuildType = "legacy",sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModeclearly decouples the CDN URL, version, and file name. This enhancement improves clarity for customers with custom domains and aligns perfectly with the PR objectives.
59-61: Enhanced Error Handling Clarity:
Updating the check from usingvoid 0toundefined(as seen in the error-handling condition on line 60) improves readability and explicitly communicates the intended check. Verify that this change maintains the same runtime behavior as before.packages/sanity-suite/public/v3/manualLoadCall/index-npm-bundled.html (3)
35-35: Version Update:
The snippet version update to"3.0.59"is consistent with the rest of the codebase and ensures that the SDK is correctly versioned.
46-50: New Variable Declarations for SDK Loading:
The new variables (window.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingMode) have been introduced to improve the modularity of the SDK URL construction. This makes it straightforward for users to update only the CDN URL if needed.
59-61: Refined Error Handling Check:
The updated condition for method invocation now usesundefinedinstead ofvoid 0to check for the existence of_methodName. This change improves clarity without altering functionality.packages/sanity-suite/public/v3/manualLoadCall/index-npm.html (2)
35-35: Version Bump Verification:
Changingwindow.RudderSnippetVersionfrom"3.0.58"to"3.0.59"meets the versioning requirements outlined in the PR.
46-50: Consistent Configuration Updates:
The insertion of new variables (window.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingMode) standardizes the SDK loading snippet across files. This consistent approach ensures that customers can easily modify the base URL while the version and file name remain fixed.packages/sanity-suite/public/v3/integrations/index-npm.html (3)
35-35: Uniform Version Update:
Upgradingwindow.RudderSnippetVersionto"3.0.59"maintains alignment with the updated SDK build across various integrations.
46-50: Standardized New SDK Variables:
The new declarations forwindow.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModefollow the established pattern. This improves maintainability and reduces ambiguity when customers adjust the CDN details.
59-61: Improved Error Handling Clarity:
The check usingundefinedfor_methodNamenow makes the intent explicit. Ensure that this minor refactor does not affect any edge-case behavior.packages/sanity-suite/public/v3/integrations/index-npm-bundled.html (3)
35-35: Consistent Versioning Update:
The update ofwindow.RudderSnippetVersionto"3.0.59"is applied consistently with other files, ensuring uniformity across the SDK integration examples.
46-50: New SDK Parameters Introduced:
The addition ofwindow.rudderAnalyticsBuildType, along withsdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingMode, enhances the clarity of the SDK’s URL construction. This explicit separation helps customers update only what is necessary (e.g., the CDN base URL) and keeps the version and file name intact.
59-61: Explicit Error Handling Improvement:
Modifying the error-handling check to useundefinedinstead ofvoid 0increases the code's self-documentation. This change should be verified to behave identically to the previous implementation.packages/sanity-suite/public/v3/index-local.html (5)
35-35: Update SDK Version Number
The snippet version has been updated to"3.0.59", which clearly communicates the current version. Ensure that any downstream consumers of this value are aware of the update.
47-49: Introduce Separate URL Components
New variablessdkBaseUrl,sdkVersion, andsdkFileNameare introduced to decouple the SDK URL into distinct parts. This improves clarity for customers switching custom domains. Confirm that the chosen values (e.g."v3"forsdkVersion) correctly reflect the intended deployment configuration.
60-61: Modernize Undefined Checks
The error handling now uses an explicitundefinedcheck (i.e.(_methodName = window[identifier][methodName]) === null || _methodName === undefined || ...) instead of
void 0. This enhances readability and aligns with modern JavaScript practices.
92-98: Clarify Global Object Retrieval
The inline function that definesgetGlobalnow uses a named function declaration. This not only improves readability but may also help debugging in environments like IE11. Please ensure cross-browser testing confirms that the global object is correctly detected.
110-110: Reference for Alternative SDK Loading
The commented-out script-loading line now reflects the updated URL construction usingsdkVersionandsdkFileName. It serves as an informative reference. Ensure that any future reactivation of this code segment uses the proper variable values.packages/sanity-suite/public/v3/index-cdn.html (4)
35-35: Update SDK Snippet Version
window.RudderSnippetVersionis now set to"3.0.59". This update is consistent with the overall versioning improvements.
47-49: Define CDN URL Variables with Placeholder
The new variables—sdkBaseUrlset to"https://cdn.rudderlabs.com",sdkVersionset to"__CDN_VERSION_PATH__", andsdkFileNameset to"rsa.min.js"—clearly decouple the URL components. The use of"__CDN_VERSION_PATH__"as a placeholder suggests that a build or deployment process will substitute the correct value. Ensure that this substitution is reliably performed in the pipeline.
60-61: Consistent Undefined Handling
As with the local index file, the error handling now explicitly checks forundefined. This consistency strengthens the code quality and readability.
68-70: Validate Modern Build Type Fallback
The try-catch block that attempts to switchrudderAnalyticsBuildTypeto"modern"remains intact. Confirm that the detection of modern JS features is sufficient for your browser support matrix, especially for legacy browsers like IE11.packages/sanity-suite/public/v3/integrations/index-local.html (4)
35-35: Update SDK Version Identifier
The SDK snippet version has been updated to"3.0.59". This helps downstream code quickly determine the new version.
47-49: Establish New URL Variables
New variablessdkBaseUrl,sdkVersion(with value"__CDN_VERSION_PATH__"as a placeholder), andsdkFileNameare now defined. This separation simplifies future updates. Please verify that the placeholder value is correctly replaced during deployment.
60-61: Maintain Consistent Undefined Check
The update to check explicitly againstundefinedin place ofvoid 0improves the code clarity.
110-110: Commented Out Script Injection Update
The commented-out script-loading line now uses the new variables. While still inactive, this serves as a useful reference for the intended URL construction.packages/sanity-suite/public/v3/integrations/index-cdn.html (5)
35-35: Reflect Updated SDK Version
The SDK snippet version is updated to"3.0.59", aligning with other parts of the codebase.
47-49: Breakout of URL Components for CDN
The introduction ofsdkBaseUrl,sdkVersion(set to"__CDN_VERSION_PATH__"), andsdkFileNameclarifies the URL structure. This approach reduces ambiguity for customers who need to customize their CDN URL. Ensure that"__CDN_VERSION_PATH__"is dynamically replaced as intended.
60-61: Enhance Undefined Checks
Switching to an explicitundefinedcheck in the error handler improves the explicitness and readability of the code.
92-98: Named getGlobal Function for Better Clarity
The refactored, namedgetGlobalfunction is easier to read and debug. Confirm that this refactor does not affect the behavior in any targeted platform.
110-110: Active Script URL Construction
The call towindow.rudderAnalyticsAddScriptnow actively constructs the URL usingsdkBaseUrl,sdkVersion,window.rudderAnalyticsBuildType, andsdkFileName. This is a clear improvement for maintainability.packages/sanity-suite/public/v3/manualLoadCall/index-local.html (3)
35-35: Upgrade SDK Version Number for Manual Load Call
The version has been updated to"3.0.59", which should be consistently reflected in all SDK load mechanisms.
47-49: Introduce Decoupled URL Variables for Manual Load
The new variable definitions (sdkBaseUrlas"https://cdn.rudderlabs.com",sdkVersionas"__CDN_VERSION_PATH__", andsdkFileNameas"rsa.min.js") improve the clarity of how the SDK URL is constructed. This structure helps customers and developers update the SDK URL with minimal friction.
60-61: Maintain Consistent Error Checks
Updating the error handling to explicitly check againstundefinedremains consistent with other snippet files. This is a good practice and aids in cross-browser consistency.packages/sanity-suite/public/v3/manualLoadCall/index-cdn.html (3)
35-35: Version Update Consistency
The snippet version has been updated to"3.0.59". This change is consistent with the overall SDK version bump and ensures that deployments pick up the new release.
60-60: Enhanced Undefined Check in Method Invocation
The updated check(_methodName = window[identifier][methodName]) === null || _methodName === undefined || _methodName.apply(window[identifier], arguments);now uses an explicit comparison to
undefinedrather than relying onvoid 0. This improves code clarity in error handling.
110-110: Updated Script URL Construction
The SDK script tag is now inserted using a URL built from the new variables:window.rudderAnalyticsAddScript("".concat(sdkBaseUrl, "/").concat(sdkVersion, "/").concat(window.rudderAnalyticsBuildType, "/").concat(sdkFileName), "data-rsa-write-key", "__WRITE_KEY__");This approach should simplify future updates, especially for customers using custom domains. Please verify that the final URL is resolving as expected in your test environments.
examples/v3/index.html (4)
11-11: Version Increment Update
window.RudderSnippetVersionis updated to"3.0.59". This aligns the example with the current release.
23-25: Separation of SDK Components
The introduction of distinct variables —
•sdkBaseUrlas"https://cdn.rudderlabs.com",
•sdkVersionas"v3", and
•sdkFileNameas"rsa.min.js"—
improves modularity in URL construction and makes future updates (especially for custom domain scenarios) easier to manage.
36-36: Explicit Undefined Check in Method Invocation
Changing the check from using a non-explicit value (such asvoid 0) to a clearundefinedcheck enhances readability and maintainability.
86-86: Dynamic Script Loading URL Construction
The final URL used in the script injection is now built dynamically (using.concaton the new variables). This ensures that the SDK load path reflects the new structure. Please confirm that this updated pattern supports both legacy and modern build types as intended.examples/v3-legacy/index.html (3)
11-11: Legacy Example Version Update
The legacy example now reflects the updatedwindow.RudderSnippetVersionset to"3.0.59", ensuring consistency with the latest SDK.
23-25: Refactored URL Variables in Legacy Example
New variables —sdkBaseUrl,sdkVersion(set to"v3"), andsdkFileName(replacingsdkName) — are introduced to simplify URL construction. Please verify that this change won’t disrupt any integrations that depend on the former hardcoded layout.
86-86: Script Loading URL Update for Legacy Build
The concatenation ofsdkBaseUrl,sdkVersion, andsdkFileNamein the URL now aligns with the modular approach. Double-check this in your legacy environments to ensure no unintended side effects occur.examples/v3-minimum-plugins/index.html (4)
11-11: SDK Version Update Applied
The snippet version is updated to"3.0.59", ensuring that the minimum plugins example is in sync with the overall SDK version.
23-25: Modular SDK URL Component Introduction
The separation intosdkBaseUrl("https://cdn.rudderlabs.com"),sdkVersion("v3"), andsdkFileName("rsa.min.js") clarifies the URL-building logic and simplifies future configuration changes.
86-86: Dynamic Script Injection
The updated script injection correctly builds the source URL with the new variables. Ensure that this logic passes in all environments, especially where the plugins functionality is critical.
98-102: Addition of Plugins Configuration
A new plugins array containing'StorageEncryption'and'XhrQueue'is now included in the load options. Confirm that these plugins are available and that their inclusion meets your integration requirements.examples/v3-legacy-minimum-plugins/index.html (4)
11-11: Consistent Version Update in Legacy Minimum Plugins Example
The updated version"3.0.59"is now reflected here, bringing this legacy example in line with the rest of the codebase.
23-25: Modularization of SDK URL Variables
The refactoring from a hardcodedsdkNameto distinct variables (sdkBaseUrl,sdkVersionwith"v3", andsdkFileName) improves clarity and maintainability. This is especially helpful for users updating their integrations.
86-86: Dynamic URL Construction for Legacy Minimum Plugins
The method for constructing the script URL now uses the new variable pattern, ensuring consistency with other examples. Make sure this does not adversely affect legacy behaviors.
98-102: Consistency in Plugins Configuration
The plugins configuration remains consistent with the minimum plugins example, which is beneficial for predictable behavior. Please verify that both'StorageEncryption'and'XhrQueue'are the intended values for plugin activation.examples/v3-beacon/index.html (3)
11-11: Update Snippet Version
The snippet version has been updated from "3.0.58" to "3.0.59" as per the PR objectives. This ensures that customers are aware of the latest SDK release.
23-25: Introduce SDK URL Components
The new variables (sdkBaseUrl,sdkVersion, andsdkFileName) decouple the CDN base URL from the version and filename. This improves clarity and maintainability—especially for customers updating custom domains.
36-36: Clarify Undefined Check in Method Dispatch
The updated check now explicitly compares againstundefined(instead of usingvoid 0), which enhances readability. Please ensure that this change is consistent with other parts of the codebase.packages/analytics-js/public/index.html (5)
13-13: Update Snippet Version Consistently
The snippet version has been updated to "3.0.59" to match other parts of the SDK. This unified versioning across the codebase avoids ambiguity.
25-27: Introduce SDK URL Components in Public Index
The addition ofsdkBaseUrl,sdkVersion, andsdkFileNamehere mirrors the changes in other files. This makes it easier for users to update only the CDN URL when needed.
38-38: Consistent Undefined Check in Method Invocation
The method invocation now explicitly checks forundefinedbefore calling the method. This clarification improves code readability and maintains consistent error handling.
69-86: Enhanced GlobalThis Polyfill
The polyfill forglobalThishas been updated with a dedicatedgetGlobalfunction that checks for bothselfandwindow. This explicit polyfill approach is clear, but you might also consider leveraging a well-tested polyfill library if cross-environment compatibility expands.
171-182: Review Environment Variables Check
The conditions that checkenvDestSDKBaseURL,envPluginsSDKBaseURL, andenvConfigUrlcompare against the string'undefined'. If these placeholders are intended to be replaced during build time, this is acceptable; however, if they can be actual JavaScriptundefinedvalues, atypeofcheck (e.g.,typeof envDestSDKBaseURL !== 'undefined') might be more robust. Please verify that these checks align with your build/replacement process.
saikumarrs
force-pushed
the
fix.loading-snippet-sdk-2942
branch
from
5ebe326 to
31321e8
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/sanity-suite/public/v3/integrations/index-npm-bundled.html (1)
46-50: New SDK Configuration Variables Added.
The introduction ofwindow.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModeenhances clarity by isolating the core SDK URL components. This will aid users employing custom domains to update only the required element without affecting the version or file name.
Consider adding inline documentation/comments for these variables to improve long-term maintainability and ease of use by other developers.packages/sanity-suite/public/v3/manualLoadCall/index-npm.html (1)
46-50: Introduced New Variables for SDK URL Construction
The addition ofsdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingModemakes the snippet’s URL components explicit and easier to update for custom domains. Consider usingconstinstead ofvarfor these variables if their values are immutable for improved code safety and readability. For example:- var sdkBaseUrl = "https://cdn.rudderlabs.com"; - var sdkVersion = "v3"; - var sdkFileName = "rsa.min.js"; - var scriptLoadingMode = "async"; + const sdkBaseUrl = "https://cdn.rudderlabs.com"; + const sdkVersion = "v3"; + const sdkFileName = "rsa.min.js"; + const scriptLoadingMode = "async";packages/sanity-suite/public/v3/manualLoadCall/index-local.html (1)
110-110: Document the New SDK URL Composition Example
The commented-out script-loading line now accurately reflects the new mechanism for constructing the SDK URL usingsdkBaseUrl,sdkVersion, andsdkFileName. While it remains commented, please verify that if re-enabled, this pattern correctly concatenates the URL segments and that the replacement for__WRITE_KEY__is handled appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
examples/angular/sample-app/src/index.html(3 hunks)examples/integrations/Ninetailed/sample-apps/app-using-v3-cdn/public/index.html(1 hunks)examples/nextjs/hooks/sample-app/src/app/layout.tsx(3 hunks)examples/nextjs/js/sample-app/src/app/layout.js(3 hunks)examples/nextjs/page-router/sample-app/src/pages/_document.tsx(3 hunks)examples/nextjs/ts/sample-app/src/app/layout.tsx(3 hunks)examples/reactjs/hooks/sample-app/public/index.html(3 hunks)examples/reactjs/js/sample-app/public/index.html(3 hunks)examples/reactjs/ts/sample-app/public/index.html(3 hunks)examples/reactjs/vite/sample-app/index.html(3 hunks)examples/v3-beacon/index.html(4 hunks)examples/v3-legacy-minimum-plugins/index.html(4 hunks)examples/v3-legacy/index.html(4 hunks)examples/v3-minimum-plugins/index.html(4 hunks)examples/v3/index.html(4 hunks)packages/analytics-js/__fixtures__/msw.handlers.ts(1 hunks)packages/analytics-js/__tests__/nativeSdkLoader.js(4 hunks)packages/analytics-js/public/index.html(5 hunks)packages/loading-scripts/rollup.config.mjs(2 hunks)packages/loading-scripts/src/index.ts(2 hunks)packages/sanity-suite/public/v3/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/index-local.html(4 hunks)packages/sanity-suite/public/v3/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/index-npm.html(3 hunks)packages/sanity-suite/public/v3/integrations/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/integrations/index-local.html(4 hunks)packages/sanity-suite/public/v3/integrations/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/integrations/index-npm.html(3 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-cdn.html(4 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-local.html(4 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-npm-bundled.html(3 hunks)packages/sanity-suite/public/v3/manualLoadCall/index-npm.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (28)
- examples/nextjs/hooks/sample-app/src/app/layout.tsx
- packages/loading-scripts/rollup.config.mjs
- examples/reactjs/vite/sample-app/index.html
- packages/analytics-js/fixtures/msw.handlers.ts
- examples/reactjs/hooks/sample-app/public/index.html
- examples/nextjs/ts/sample-app/src/app/layout.tsx
- examples/reactjs/js/sample-app/public/index.html
- packages/sanity-suite/public/v3/index-npm-bundled.html
- packages/analytics-js/public/index.html
- packages/analytics-js/tests/nativeSdkLoader.js
- examples/v3-minimum-plugins/index.html
- examples/v3-beacon/index.html
- packages/loading-scripts/src/index.ts
- packages/sanity-suite/public/v3/manualLoadCall/index-npm-bundled.html
- examples/nextjs/page-router/sample-app/src/pages/_document.tsx
- examples/reactjs/ts/sample-app/public/index.html
- examples/nextjs/js/sample-app/src/app/layout.js
- packages/sanity-suite/public/v3/index-local.html
- packages/sanity-suite/public/v3/index-npm.html
- examples/integrations/Ninetailed/sample-apps/app-using-v3-cdn/public/index.html
- packages/sanity-suite/public/v3/integrations/index-local.html
- examples/v3/index.html
- examples/angular/sample-app/src/index.html
- packages/sanity-suite/public/v3/integrations/index-npm.html
- examples/v3-legacy-minimum-plugins/index.html
- packages/sanity-suite/public/v3/integrations/index-cdn.html
- packages/sanity-suite/public/v3/index-cdn.html
- examples/v3-legacy/index.html
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Bundle size checks
- GitHub Check: Unit Tests and Lint
- GitHub Check: Security and code quality checks
🔇 Additional comments (14)
packages/sanity-suite/public/v3/integrations/index-npm-bundled.html (2)
35-35: Version Update: RudderSnippetVersion Bump.
The snippet version has been correctly updated to "3.0.60", which aligns with the PR’s objective of version incrementing.
60-60: Improved Undefined Check in Error Handling.
Replacing the check fromvoid 0toundefinedincreases the readability and clarity of the error handling logic. This adjustment is in line with modern JavaScript best practices.packages/sanity-suite/public/v3/manualLoadCall/index-npm.html (2)
35-35: Updated RudderSnippetVersion
The snippet version is correctly updated to "3.0.60". Ensure that all downstream integrations and documentation reflect this change.
60-60: Enhanced Error Handling Clarity
Changing the check from usingvoid 0to explicitly comparing againstundefinedimproves code clarity. This makes it immediately understandable that the code verifies the existence of the method before callingapply().packages/sanity-suite/public/v3/manualLoadCall/index-cdn.html (6)
35-35: Update RudderSnippetVersion for Consistency
The snippet version has been updated from"3.0.58"to"3.0.60", which ensures consistency across the SDK and aligns with the version bump required in the PR objectives.
47-49: Separate SDK URL Components for Clarity
Introducing the distinct variablessdkBaseUrl,sdkVersion, andsdkFileNameimproves clarity. Customers can now update only the CDN URL while the version and file name remain intact. This aligns perfectly with the PR objectives of removing ambiguity in URL construction.
60-60: Simplify Undefined Check in Error Handling
The error handling logic now checks forundefinedinstead of usingvoid 0. This change makes the code more readable and adheres to modern JavaScript best practices.
92-98: Robust GlobalThis Polyfill Implementation
The updated polyfill using thegetGlobalfunction now checks forself(for web workers) andwindow(for browsers) to determine the global object. This ensures better compatibility—especially for older browsers like IE11—by accurately polyfillingglobalThiswhen undefined.
103-107: Conditional Definition of globalThis
By utilizingObject.definePropertyto setglobalThison the global object when it isn’t defined, this code ensures a clean and controlled definition of the global object across environments.
110-110: Dynamic Construction of SDK URL
The script URL is now dynamically constructed by concatenatingsdkBaseUrl,sdkVersion,window.rudderAnalyticsBuildType, andsdkFileName. Please verify that this URL structure accurately supports custom domains and maintains the intended versioning behavior.packages/sanity-suite/public/v3/manualLoadCall/index-local.html (4)
35-35: Update RudderSnippetVersion to "3.0.60"
The snippet version has been correctly bumped from "3.0.58" to "3.0.60" as per the requirements, ensuring consistency across integrations.
47-49: Introduce Explicit SDK URL Variables
Splitting the CDN URL into distinct variables—sdkBaseUrl,sdkVersion, andsdkFileName—improves clarity and makes it easier for customers to update only the necessary parts (e.g., when using custom domains). Please ensure that the placeholder value"__CDN_VERSION_PATH__"forsdkVersionis properly replaced during the build or deployment process.
60-60: Refactor SDK Method Error Handling
The update replacingvoid 0with an explicitundefinedimproves clarity in the condition checking before invoking the SDK’s methods. Double-check that this change does not alter the intended short-circuit behavior in edge cases.
92-107: Enhance Global Object Detection & PolyfillglobalThis
The newly definedgetGlobalfunction, along with the use ofObject.definePropertyto polyfillglobalThis, provides a robust fallback for environments (e.g., IE11) that lack native support. This change will contribute to improved cross-browser compatibility.
PR Description
I've updated the SDK loading snippet to split the variables that form the final URL of the core SDK.
sdkBaseUrl- refers to the CDN URL without the versionsdkVersion- refers to the SDK versionsdkFileName- refers to the SDK file name on the CDNThis will be helpful for the customers who setup custom domains and have to update the snippet to just replace RS CDN URL with theirs, leaving aside the version and the file name.
Linear task (optional)
https://linear.app/rudderstack/issue/SDK-2942/update-sdk-loading-snippet-to-decouple-the-sdk-major-version-and
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
window.rudderAnalyticsBuildType,sdkBaseUrl,sdkVersion,sdkFileName, andscriptLoadingMode.Refactor
Bug Fixes
undefineddirectly instead of usingvoid 0.