[WIP] feat(optimizely): Expose Edge API to track Edge experiments #479

patrickshih-optimizely · 2020-06-04T23:52:55Z

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.

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

patrickshih-optimizely · 2020-06-04T23:55:31Z

integrations/optimizely/lib/index.js

+var push = require('global-queue')('optimizely', { wrap: false });
+var pushEdge = require('global-queue')('optimizelyEdge', { wrap: false });


Tried to do a more succinct variable assignment

var push = window.optimizelyEdge ? require('global-queue')('optimizelyEdge', { wrap: false }) : require('global-queue')('optimizely', { wrap: false });

But it wasn't passing the unit test I wrote? Would like to hear ideas for testing to get ^ in.

Hmm, I'm guessing these import statements are executed once on the test page before the mocha suite.. So depending on whether window.optimizelyEdge is defined, the push var would statically refer to optimizely or optimizelyEdge, causing tests to fail for optimizelyEdge or optimizely respectively.

Given that window.optimizelyEdge probably isn't defined at the beginning, pushes probably all end up going to optimizely, causing the optimizelyEdge tests to fail.

Given that window.optimizelyEdge probably isn't defined

Yup. I'll keep this for now so we can write test cases for Edge. Makes implementing slightly more clunky, but still readable

nchilada

Kudos for getting up to speed on this! But I suspect we're not done yet. This PR alter the use of push({type: 'event', ...}), but it does not alter the use of push({type: 'addListener', ...}) or the get APIs. Given that window.optimizely.push technically works on Edge pages (though window.optimizelyEdge.push is the only official supported API), this PR will presumably add the most value once it addresses those remaining API calls.

nchilada · 2020-06-05T21:52:05Z

integrations/optimizely/test/index.test.js

        analytics.stub(window.optimizely, 'push');
      });

+      describe('when the experiment running is from Edge', function() {


Suggested change

describe('when the experiment running is from Edge', function() {

context('when Optimizely Edge is implemented on the page', function() {

or

Suggested change

describe('when the experiment running is from Edge', function() {

context('when Optimizely Performance Edge is implemented on the page', function() {

To help readers understand that this is a variation on the theme, maybe move this describe/context such that it follows it('should send an event')? If that makes sense

nchilada · 2020-06-05T21:59:02Z

integrations/optimizely/test/index.test.js

+          });
+        });
+      });
+


Hmm, looking at all the tags tests below, I'm guessing folks will expect to be able to track tags.

A tags property does seem to be documented in the Edge API but I thought we'd decided not to support them...let's chat offline to see if there's anything that needs to be done to support this?

Okay, it looks like we haven't yet added support for visual tags, but custom event tags (and maybe even global tags) passed via the API are probably supported

nchilada · 2020-06-05T22:00:04Z

integrations/optimizely/package.json

    "karma-sauce-launcher": "^2.0.2",
    "karma-spec-reporter": "^0.0.32",
    "karma-summary-reporter": "^1.6.0",
+    "lerna": "^3.15.0",


Seems to be unnecessary, but I'll let the Segment folks weigh in

TBH, I believe it's a mistake on my end

nchilada · 2020-06-05T22:20:13Z

integrations/optimizely/lib/index.js

+      type: 'integration',
+      OAuthClientId: '5360906403'
+    });
+  }


Interesting, this seems to be documented here, but was only ever active in Classic Web (see here and here) and hasn't been ported to X Web/Edge. Doesn't hurt I guess

+1 cool, thanks I should've done some due diligence and also checked the codebase! I'll get rid of pushing this if it's edge

No worries! I'm not actually sure what to do about this. We no longer have folks actively developing integrations, but maybe if we get back there one day, it might be useful to be collect this data.

I think we could go either way (continue to track, or not) as long as we do the same thing regardless of what mode we're in (Classic Web, X Web, Edge).

nchilada · 2020-06-05T22:25:06Z

integrations/optimizely/HISTORY.md

+3.6.0 / 2020-06-04
+==================
+
+  * Exposed `window.optimizelyEdge` so customers with Performance Edge are able to track Performance Edge related experiments in Segment.


Suggested change

* Exposed `window.optimizelyEdge` so customers with Performance Edge are able to track Performance Edge related experiments in Segment.

* Track conversion events using `window.optimizelyEdge` instead of `window.optimizely` if Optimizely Performance Edge is running on the page.

based on what I'm currently seeing in the PR

nchilada · 2020-06-05T22:34:48Z

integrations/optimizely/lib/index.js

+var push = require('global-queue')('optimizely', { wrap: false });
+var pushEdge = require('global-queue')('optimizelyEdge', { wrap: false });


Hmm, I'm guessing these import statements are executed once on the test page before the mocha suite.. So depending on whether window.optimizelyEdge is defined, the push var would statically refer to optimizely or optimizelyEdge, causing tests to fail for optimizelyEdge or optimizely respectively.

Given that window.optimizelyEdge probably isn't defined at the beginning, pushes probably all end up going to optimizely, causing the optimizelyEdge tests to fail.

nchilada · 2020-08-27T17:56:31Z

I think this has been superseded by #483

patrickshih-optimizely · 2020-08-27T18:03:36Z

Yup, PR is closed

expose edge

b6c7d7c

patrickshih-optimizely commented Jun 4, 2020

View reviewed changes

Merge branch 'master' into patrickshih/expose-edge

9932c48

patrickshih-optimizely marked this pull request as ready for review June 5, 2020 18:31

nchilada suggested changes Jun 5, 2020

View reviewed changes

nchilada mentioned this pull request Jun 12, 2020

refactor: Clean up the Optimizely integration #481

Open

a record of progress, will be rebased on top of nikhil's branch

19b11c6

patrickshih-optimizely changed the title ~~feat(optimizely): Expose Edge API to track Edge experiments~~ [WIP] feat(optimizely): Expose Edge API to track Edge experiments Jun 24, 2020

patrickshih-optimizely closed this Jun 24, 2020

patrickshih-optimizely deleted the patrickshih/expose-edge branch June 24, 2020 22:24

nchilada mentioned this pull request Aug 13, 2020

Release #481 (refactor: Clean up the Optimizely integration) #501

Closed

		var push = require('global-queue')('optimizely', { wrap: false });
		var pushEdge = require('global-queue')('optimizelyEdge', { wrap: false });

	describe('when the experiment running is from Edge', function() {
	context('when Optimizely Edge is implemented on the page', function() {

	describe('when the experiment running is from Edge', function() {
	context('when Optimizely Performance Edge is implemented on the page', function() {

	* Exposed `window.optimizelyEdge` so customers with Performance Edge are able to track Performance Edge related experiments in Segment.
	* Track conversion events using `window.optimizelyEdge` instead of `window.optimizely` if Optimizely Performance Edge is running on the page.

[WIP] feat(optimizely): Expose Edge API to track Edge experiments #479

[WIP] feat(optimizely): Expose Edge API to track Edge experiments #479

Uh oh!

Conversation

patrickshih-optimizely commented Jun 4, 2020

Uh oh!

patrickshih-optimizely Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchilada commented Aug 27, 2020

Uh oh!

patrickshih-optimizely commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

patrickshih-optimizely Jun 4, 2020 •

edited

Loading

nchilada Jun 5, 2020 •

edited

Loading

nchilada Jun 5, 2020 •

edited

Loading