analytics.js-integrations
analytics.js-integrations copied to clipboard
refactor: Clean up the Optimizely integration
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 tonewActiveCampaign
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
, andsendCampaignData
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
, andchai
asdevDependencies
and use them throughout the tests. The preexisting tests used things likeanalytics.stub
,analytics.called
, andanalytics.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!
@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 Thanks for this! I'm going to work on releasing this shortly.
@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 @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).
@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.
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 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!