analytics.js-integrations icon indicating copy to clipboard operation
analytics.js-integrations copied to clipboard

refactor: Clean up the Optimizely integration

Open nchilada opened this issue 4 years ago • 7 comments

What does this PR do?

Product changes

  • 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 but it couldn't possibly have worked.

Bug fixes

  • Don't try to track future inactive campaigns. The listener configured by registerFutureActiveCampaigns collects null campaign decisions in addition to active campaign decisions; until this PR, such campaigns were being passed to newActiveCampaign where they were throwing (asynchronous and thus inconsequential) errors.
  • If there is an effective referrer for the current page load, include it when tracking future campaigns and not just when tracking current campaigns.

Other refactoring

  • Stop using dependency injection (the referrerOverride, sendExperimentData, and sendCampaignData hooks on the left-hand side of the diff. It looks like that pattern was copy-and-pasted from here or here; in any case, it's confusing and unnecessary.
  • Define setRedirectInfo as a stateful method.
  • Test initWebIntegration as an isolated unit instead of invoking it as part of analytics.initialize. Ideally we'd do the same for sendWebDecisionToSegment`, but I ran out of energy.
  • Explicitly install lodash, sinon, and chai as devDependencies and use them throughout the tests. The preexisting tests used things like analytics.stub, analytics.called, and analytics.deepEqual but those are undocumented and seem to be less full-featured. As someone who doesn't have access to internal Segment documentation, those functions are very difficult to work with.

Are there breaking changes in this PR?

  • Technically yes, but effectively no, since the removed functionality is relevant to Optimizely products that haven't been supported in a long time.

Any background context you want to provide?

  • Our Classic Web product, the predecessor to X Web, is long gone.
  • We recently launched Optimizely Performance Edge, which may be implemented instead of Optimizely Web on any given page. We'll extend analytics.js-integrations to add support for it in a followup pull request.

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

No. On the contrary, the "Custom Experiment Properties" setting described here is obsolete now that Optimizely Classic is gone (and was also incorrectly documented or implemented), and the "Custom Campaign Properties" setting can also be un-documented because it too was incorrectly implemented and is now being removed.

Links to helpful docs and other external resources

I've linked to some API docs from JSDoc comments!

nchilada avatar Jun 12 '20 23:06 nchilada

@patrickshih-optimizely you may want to rebase #479 on top of this. I'll leave it up to you! One of us will definitely have merge conflicts 😆

nchilada avatar Jun 12 '20 23:06 nchilada

@nchilada Thanks for this! I'm going to work on releasing this shortly.

tysonmote avatar Aug 10 '20 20:08 tysonmote

@nchilada It looks like this PR introduces some new lint warnings in our CI. Do you think you could fix these up? (Sorry that this isn't clear -- I'm not sure why our CircleCI setup isn't able to run successfully with external branches, yet.)

/workdir/integrations/optimizely/lib/index.js
--
  | 292:15  warning  'state' is already declared in the upper scope  no-shadow
  |  
  | /workdir/integrations/optimizely/test/index.test.js
  | 106:11  warning  for..in loops iterate over the entire prototype chain, which is virtually never what you want. Use Object.{keys,values,entries}, and iterate over the resulting array  no-restricted-syntax
  | 170:7   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 666:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 690:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 720:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 747:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  | 774:9   warning  'executeAsyncTest' was used before it was defined                                                                                                                      no-use-before-define
  |  
  | ✖ 8 problems (0 errors, 8 warnings)

tysonmote avatar Aug 10 '20 22:08 tysonmote

@tysonmote @gpsamson thank you for the reviews and sorry for the late update! I've pushed some lint fixes; hopefully they help. I'm heading out of office through next week but @patrickshih-optimizely and I can check in to make sure you don't need anything to more for this PR.

BTW would you all be in charge of any necessary integration testing? I saw a link to this README but I don't have a lot of context (I've never directly used Segment myself).

nchilada avatar Aug 13 '20 02:08 nchilada

@patrickshih-optimizely and @nchilada — Thank you for your patience! We are planning to merge this PR and https://github.com/segmentio/analytics.js-integrations/pull/483 on May 3, 2021. Let us know if anything has changed in the past (many) months.

jenskene avatar Apr 21 '21 22:04 jenskene

Hi @jenskene ! Thanks for getting back to us! @nchilada had since moved on from Optimizely, feel free to direct any questions about this PR to me.

I scanned this PR once again and this one looks good. Mainly, because most of the changes in this PR is removing old functionality that Optimizely does not support anymore (specifically, our Classic product and things w.r.t Classic)

patrickshih-optimizely avatar Apr 22 '21 02:04 patrickshih-optimizely

@patrickshih-optimizely thanks very much for taking over! @jenskene yup I left Optimizely last year and stopped getting notifications. Sorry for not posting an update in either PR!

nchilada avatar Apr 23 '21 15:04 nchilada