-
Notifications
You must be signed in to change notification settings - Fork 52
feat: Expose Rokt launcher config options #1033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Expose Rokt launcher config options #1033
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for passing Rokt Web SDK launcher options through the Kit SDK, alongside existing sandbox settings.
- Introduces a nested
IRoktOptions
type to bundlemanagerOptions
(sandbox) and newlauncherOptions
. - Updates
RoktManager
to store and exposelauncherOptions
during initialization. - Expands test coverage for both provided and omitted launcher options in
roktManager.spec.ts
.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
test/jest/roktManager.spec.ts | Updated tests to wrap sandbox under managerOptions and added cases for launcherOptions . |
src/sdkRuntimeModels.ts | Exposed launcherOptions in SDKInitConfig for consumer configuration. |
src/roktManager.ts | Added IRoktOptions with launcherOptions , updated init logic and interfaces. |
Comments suppressed due to low confidence (2)
test/jest/roktManager.spec.ts:250
- [nitpick] The test description says 'empty launcher options' but asserts
undefined
; consider updating the wording to 'undefined launcher options' for clarity.
it('should initialize the manager with empty launcher options when not provided', () => {
src/roktManager.ts:53
- [nitpick] The
launcherOptions
property onIRoktKit
is only used internally byRoktManager
; consider removing it from the publicIRoktKit
interface to avoid confusion.
launcherOptions?: Dictionary<any>;
8e5e823
to
31367de
Compare
{ | ||
managerOptions: { sandbox: true }, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test from the past that shows if sandbox is passed into the selectPlacements call that it will override MP's isDevelopmentMode
boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. We treat both createLauncher
and selectPlacements
sandbox flags as pass throughs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, just minor comments
957f614
to
c411fa7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR exposes Rokt launcher configuration options so that the Kit SDK can pass these options through to the Rokt Web SDK. Key changes include:
- Updating tests in both identity and RoktManager suites to validate new configuration and behavior.
- Adjusting SDK and Manager interfaces and initialization parameters to support launcherOptions.
- Modifying mp-instance and identity logic to integrate with the updated RoktManager API.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/src/tests-identity.ts | Added tests for Rokt Manager behavior and new identify helper functions. |
test/jest/roktManager.spec.ts | Updated tests to verify proper initialization of launcher and manager options. |
src/sdkRuntimeModels.ts | Updated SDKInitConfig to include launcherOptions. |
src/roktManager.ts | Updated options handling to use the new IRoktOptions structure and store launcherOptions. |
src/mp-instance.ts | Modified RoktManager initialization to pass managerOptions and launcherOptions. |
src/identity.js | Added a check to update currentUser from Identity when the RoktManager is ready. |
@@ -1677,6 +1677,10 @@ export default function Identity(mpInstance) { | |||
} | |||
mpInstance._Store.isInitialized = true; | |||
|
|||
if (mpInstance._RoktManager.isReady()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if it's not ready yet because identity returns faster than rokt manager gets initialized, this means there still won't be a current user right/. Is there a way to queue setting the current user for when it is ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually is more of a check to see if the Rokt Kit is being used at all. We could potentially create a new method called isConfigured
or isActive
, but I felt like that might be unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it. yea seems like it's more readable/grokable if you go with an approach like isActive. I think you can make it a tech debt ticket.
Food for thought - If we are passing sandbox to createLauncher now, is there a reason to include it in the |
Co-authored-by: Robert Ing <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR exposes configuration options for Rokt's launcher, enabling the Kit SDK to pass these options to the Rokt Web SDK. Key changes include:
- Extending the SDK's configuration by introducing a new "launcherOptions" parameter.
- Adding tests to validate the behavior of Rokt Manager with respect to user identity and launcher options.
- Updating various modules (including sdkRuntimeModels, mp-instance, and identity) to properly propagate and utilize the new configuration options.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
test/src/tests-identity.ts | Added tests for validating the behavior when a user’s email is identified and when identity calls fail. |
test/jest/roktManager.spec.ts | Updated tests to verify initialization of launcher options and sandbox mode. |
src/sdkRuntimeModels.ts | Added a new optional launcherOptions field to the SDK initialization configuration. |
src/roktManager.ts | Refactored the RoktManager initialization to support new launcherOptions and managerOptions. |
src/mp-instance.ts | Updated initialization logic to pass new Rokt options to RoktManager. |
src/identity.js | Modified identity flow to set currentUser in RoktManager when ready. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment was actually that I didn't think we needed RoktManager.sandbox at all because we aren't using it anywhere, right? What is its current purpose?
Ignore me!
🎉 This PR is included in version 2.41.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Instructions
development
Summary
Testing Plan
window.mParticle.config
and verify that they appear in the Rokt Web SDKselectPlacements
call.Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)