-
Notifications
You must be signed in to change notification settings - Fork 141
feat(optimizely): Expose Edge API to track Edge experiments #483
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
base: master
Are you sure you want to change the base?
feat(optimizely): Expose Edge API to track Edge experiments #483
Conversation
* Don't attempt to interface with Optimizely Classic Web, since Classic is finally dead * Prepare to support Optimizely Edge, an alternative to Optimizely Web * Drop all references to customCampaignProperties. It seems to have been documented [here](https://segment.com/docs/connections/destinations/catalog/optimizely-web/#settings) but it couldn't possibly have worked. * Generally refactor the code and tests.
|
|
@patrickshih-optimizely I'm going to work on getting this merged and released. Can you rebase on #481? I hit some merge conflicts and I think you'd be best positioned to resolve them. Thanks! |
|
@tysonmote Yes, I'll get to rebasing, thanks for the heads up! |
72babb5 to
8ba43dd
Compare
8ba43dd to
df45713
Compare
df45713 to
394547a
Compare
| } catch (e) { | ||
| done(e); | ||
| } | ||
| describe('#initialize on Web', function() { |
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.
A little confusing to tell in the diff, but I moved down both #initialize with Web and #initialize with Edge to the end of the test suite. #initialize with Edge causes #sendWebDecisionToSegment tests to completely fail, but I knew that tests were passing individually. This leads me to believe that there is something (unsure if it already existed or if I introduced it) in the testing module where variables/data aren't being reset before each test.
Putting all the test to the bottom passes all of them now. Maybe someone on the Segment side may have an idea why this is happening?
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.
I'm happy to help look into this too, if that's helpful. I might have some context from my PR.
|
@tysonmote, I rebased and cleaned up the code to make it more aligned with #481's changes! I asked a question that I was wondering you may might have some understanding on. |
|
Hey Optimizely team, quick FYI that our CI is failing with |
|
Hey @nchilada @patrickshih-optimizely, just wanted to follow up here. It looks as if a number of modules are now imported from Optimizely private jfrog. Curious your thoughts on the best path forward. I can simply replace the imported modules with the corresponding public packages. Thanks! |
|
Hi @brennan, there are some recent updates from Optimizely which requires some update in this PR. Sorry for not communicating this earlier; I'll be working on updating this PR this sprint (until 10/2). Once I finish making the changes, I'll ping you in this PR for next steps. As for the modules imported via private jfrog, I'm not sure if there's another way forward. Seems like your idea with using corresponding public packages would work 👍 |
|
@brennan, I've updated this PR to reflect the most up-to-date use for Optimizely Edge. This is ready to be reviewed and merged. I'm also curious if there are any updates with #481, since this PR is build on top of #481. Will #481 be merged first? Heads up @nchilada has since left Optimizely, so if there are any info or questions originally directed to him or Optimizely, please tag me! |
|
@brennan, any updates on getting this PR in? |
|
Hey @patrickshih-optimizely. Thanks for following up! I'll take a closer look at this PR today. Note, we're releasing to a limited pool of customers only to validate the PR, then will release to a wider audience. |
|
Hi @brennan, let me know if there's any more updates or aspects of this PR that I can address! We're hoping to get this in soon. |
|
Hey @patrickshih-optimizely, and apologies for the delay. I should have time to look this over this week. Note that we're not planning on a general release, however, until a few customers are able to beta test. A few have already agreed to do so. |
|
Great, that sounds good. Thanks @brennan ! |
|
@patrickshih-optimizely Are you able to look into reverting the |
|
Hi @brennan, I reverted the yarn.lock file, but kept the dependencies we added in |
|
Hey @brennan, I understand that y'all will have customers beta this branch first before release. I was wondering if you could let me know what that timeline may look like (for example, details about # of customers, or how long a beta may last). Also, are there other ways of communication that would be more appropriate for updates? |
|
Hey @patrickshih-optimizely ! I'm a PM at Segment, and I want to follow up on our work to support the Edge API. We had one customer who was really keen to use it, but they have had a change in team leadership - and in priority. I am checking with our Customer Success Managers to see if there are other customers who are interested in testing out a beta integration. Until we can find a customer willing to try it out and provide feedback, we are on pause. |
|
Hi @jenskene ! I'm the PM from the Optimizely side and curious as to which customer declined the beta? I have a few customers who have raised this in the past so I can definitely reach out to them as well. Thanks! |
|
@meiluo7, @patrickshih-optimizely and @nchilada — Thank you for your patience! We have a customer who is going to test Optimizely Edge, and we want to merge this PR in our next sprint, approx May 3, 2021. I know it has been a while, so let us know if anything has changed. We are also to merge #481. Thank you! |
|
thank you for the update @jenskene! I looked over this PR and everything looks good. We were wondering what the plan is for the customer to test this integration, and if there's any way for us to correspond with the customer and Segment. Specifically, if any questions or concerns come up, is there a better way for us to communicate? |
|
hi @jenskene! any updates on merging this PR? Thanks! |
|
Hi @jenskene! would love to hear any updates on whether this PR will be merged within your sprint. Thank you! |
What does this PR do?
Wraps all push calls with a conditional, which if window.optimizelyEdge (the edge API) exists on a page, it'll push to window.optimizelyEdge instead of window.optimizely.
PR is built on top of #481, as there would be much more unnecessary complications to add Edge to the current master. However, one can still view the differences here.
Are there breaking changes in this PR?
No, we still track and push to window.optimizely in Web.
Any background context you want to provide?
Allows Performance Edge customers to track Performance Edge experiments now.
Is there parity with the server-side/android/iOS integration components (if applicable)?
N/A
Does this require a new integration setting? If so, please explain how the new setting works
N/A
Links to helpful docs and other external resources