Skip to content
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

Pde/multiple optimizely #1410

Merged
merged 24 commits into from
May 11, 2022
Merged

Pde/multiple optimizely #1410

merged 24 commits into from
May 11, 2022

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Apr 21, 2022

Trying to make it easier to have two optimizely client instances at the same time

@jcger
Copy link
Contributor Author

jcger commented Apr 29, 2022

missing tests and documentation

@alextremp alextremp marked this pull request as ready for review May 9, 2022 10:06
@alextremp alextremp self-assigned this May 9, 2022
Copy link
Contributor

@rmoralp rmoralp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about cover it with some testing?

### Multiple Optimizely Adapters

Meant to exist if you need more than one decision taking optimizely sdk.
Integration with segment will only work for the first one so it will be necesary to configure segment so it forwards the events to the other optimizely destinations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we should do in Segment config to enable it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated with more specific info

* @param {object} optimizely test purposes only, optimizely sdk
* @param {object} param.options datafile options https://docs.developers.optimizely.com/full-stack/docs/initialize-sdk-javascript-node
* @param {object} param.optimizely test purposes only, optimizely sdk
* @param {function} param.eventDispatcher https://docs.developers.optimizely.com/full-stack/docs/initialize-sdk-javascript-node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {function} param.eventDispatcher https://docs.developers.optimizely.com/full-stack/docs/initialize-sdk-javascript-node
* @param {function} param.eventDispatcher https://docs.developers.optimizely.com/full-stack/docs/configure-event-dispatcher-javascript-node

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated thanks!

* @return {{variation: string}}
*/
export default function useExperiment({
experimentName,
attributes,
trackExperimentViewed,
queryString // for test purposes only
queryString,
adapterId
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is adapterId required?
Could you add some examples about useFeature and useExperiment when using multiple adapters?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#adapters = {}

constructor(optimizelyAdapters) {
defaultAdapterId = Object.keys(optimizelyAdapters)[0] // first adapter will be the defaultAdapter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If optimizelyAdapters object keys are adapterId's, why not find for default one first?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adapterIds are not required to have a default key, you could pass:

const multiple = new MultipleOptimizelyAdapter({
   adapter1: firstAdapter,
   adapter2: secondAdapter,
})

In this case, the defaultAdapterId would be adapter1 so when using PDE without passing any adapterId in the calls, adapter1 will be used.

Copy link
Contributor

@rmoralp rmoralp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

@alextremp alextremp merged commit e6db09c into master May 11, 2022
@alextremp alextremp deleted the pde/multiple-optimizely branch May 11, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants