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

Update Opticks to support Optimizely's JS SDK v5.3.2 #81

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,24 +14,18 @@ The library consists of two related concepts:

At the heart of our experimentation framework is the `toggle` function.

- `toggle` toggles that switch between multiple experiment variants (a/b/c/...)
- `booleanToggle` toggles that turn functionality on or off (feature flags)
A toggle allow you to switch between multiple experiment variants (a/b/c/...)
vlacerda marked this conversation as resolved.
Show resolved Hide resolved
and also turn functionality on or off (feature flags)

It can be used in a variety of ways:

1. Reading the value of the toggle (boolean, or a/b/c for multi toggles )
1. Execute code or for a variant of a multi toggle
1. Execute code when a boolean toggle is on

We use React at FindHotel and some of the code examples use JSX, but the code
We use React at vio.com and some of the code examples use JSX, but the code
and concept is compatible with any front-end framework or architecture.

The `booleanToggle` is the simplest toggle type to use for feature flagging, but
it's also the least flexible. As of version 2.0 `toggle` is the default and
recommended for both a/b/c experiments and feature flags. If you're only ever
interested in feature flags, read more about [Boolean
Toggles](docs/booleanToggles.md).

### Opticks vs other experimentation frameworks

The main reason for using the Opticks library is to be able to clean your code
Expand Down Expand Up @@ -64,9 +58,9 @@ See the [Optimizely integration documentation](docs/optimizely-integration.md).

## Toggles

Toggles can be used to implement a/b/c style testing, instead of on/off values
as with `booleanToggle`, we specify multiple variants of which one is active at
any time. By convention the variants are named `a` (control), `b`, `c` etc.
Toggles can be used to implement a/b/c style testing and on/off values as well.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Toggles can be used to implement a/b/c style testing and on/off values as well.
Toggles can be used to implement a/b/c style MVT testing and on/off feature flags as well.

We specify multiple variants of which one is active at any time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
We specify multiple variants of which one is active at any time.
We specify multiple variants of which only one is active at any time.

By convention the variants are named `a` (control), `b`, `c` etc.

### Reading values

Expand Down Expand Up @@ -133,7 +127,7 @@ experimentId, then the values for `a`, `b`, etc.
For instance:

```
// simple boolean switch: (you could use a BooleanToggle as well)
// simple boolean switch
const shouldDoSomething = toggle('foo', false, true)

// multiple variants as strings
Expand Down
4 changes: 2 additions & 2 deletions packages/lib/package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

Remember to bump the version of the opticks package here.

Copy link
Collaborator

@dale-french dale-french Jun 6, 2024

Choose a reason for hiding this comment

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

No need to do this manually - you can run npx changeset from the root dir which will generate the changeset. Once this PR is merged, another PR will be opened automatically with the version bump + changelog

Example

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vlacerda let me know if you face any issues generating the changeset

Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@
"author": "Jop de Klein",
"license": "ISC",
"peerDependencies": {
"@optimizely/optimizely-sdk": "~4.4.3"
"@optimizely/optimizely-sdk": "5.3.2"
},
"devDependencies": {
"@optimizely/optimizely-sdk": "~4.4.3",
"@optimizely/optimizely-sdk": "5.3.2",
"@types/jest": "^29.4.0",
"jest": "^29.4.0",
"ts-jest": "^29.0.5",
Expand Down
58 changes: 0 additions & 58 deletions packages/lib/src/core/booleanToggle.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

I still see some references to booleanToggle:

  • Documentation: docs/booleanToggles.md and docs/dead-code-removal.md
  • Logic: packages/lib/src/integrations/simple.ts
  • Type: BooleanToggleType

Can we clean this up as well? If not, let's mark them as deprecated and create a follow-up story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Good catch. I cleaned them up. I think I managed to do so .

This file was deleted.

24 changes: 0 additions & 24 deletions packages/lib/src/core/booleanToggle.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,18 @@ export const createInstanceMock = jest.fn(() => ({
notificationCenter: {
addNotificationListener: addNotificationListenerMock
},
activate: activateMock
activate: activateMock,
createUserContext: optimizelyUserContextMock
}))

export const decideMock = jest.fn((toggleKey) => ({
enabled: toggleKey === "foo"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
enabled: toggleKey === "foo"
enabled: toggleKey === 'foo'

}))
export const optimizelyUserContextMock = jest.fn((userId, attributes) => ({
decide: decideMock
}))


export const isFeatureEnabledMock = jest.fn((toggleId) => toggleId === 'foo')

export const getEnabledFeaturesMock = jest.fn((userId, attributes) => {
Expand All @@ -38,8 +47,12 @@ export const activateMock = jest.fn((toggleId, userId) => {
return shouldReturnB && 'b'
})

const originalModule = jest.requireActual('@optimizely/optimizely-sdk')

const mock = {
...originalModule,
createInstance: createInstanceMock
}


export default mock
56 changes: 15 additions & 41 deletions packages/lib/src/integrations/optimizely.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,12 @@ import {
setUserId,
setAudienceSegmentationAttributes,
resetAudienceSegmentationAttributes,
booleanToggle,
toggle,
forceToggles,
getEnabledFeatures
} from './optimizely'

// During the tests:
// for booleanToggle 'foo' yields true and 'bar' yields false, unless forced
// for toggle 'foo' yields 'b' and 'bar' yields 'a', unless forced
import datafile from './__fixtures__/dataFile'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update this datafile to match the new configuration format?


Expand All @@ -26,7 +24,11 @@ import Optimizely, {
// @ts-expect-error
getEnabledFeaturesMock,
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this mock is not used so we can remove the import (and maybe even the mock declaration).

// @ts-expect-error
activateMock
activateMock,
// @ts-expect-error
decideMock,
// @ts-expect-error
optimizelyUserContextMock
} from '@optimizely/optimizely-sdk'

// Re-used between toggle test suites
Expand Down Expand Up @@ -123,9 +125,6 @@ describe('Optimizely Integration', () => {
expect(() => toggle('foo')).toThrow(
'Opticks: Fatal error: user id is not set'
)
expect(() => booleanToggle('foo')).toThrow(
'Opticks: Fatal error: user id is not set'
)
})
})

Expand All @@ -140,25 +139,18 @@ describe('Optimizely Integration', () => {
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
expect(toggle('foo')).toEqual('b')
expect(toggle('bar')).toEqual('a')
expect(booleanToggle('foo')).toEqual(true)
expect(booleanToggle('bar')).toEqual(false)
})
*/

it('Forwards toggle reading and audienceSegmentationAttributes to Optimizely', () => {
toggle('foo', 'a', 'b', 'c')
expect(activateMock).toHaveBeenCalledWith('foo', 'fooBSide', {})
toggle('foo')
expect(isFeatureEnabledMock).toHaveBeenCalledWith(
'foo',
'fooBSide',
{}
expect(decideMock).toHaveBeenCalledWith(
'foo'
)
booleanToggle('foo')
expect(isFeatureEnabledMock).toHaveBeenCalledWith(
'foo',
'fooBSide',
{}
expect(optimizelyUserContextMock).toHaveBeenCalledWith(
'fooBSide', {}
)
})
})
Expand All @@ -182,16 +174,11 @@ describe('Optimizely Integration', () => {
deviceType: 'mobile',
isLoggedIn: false
})
})

it('Forwards toggle reading and audienceSegmentationAttributes to Optimizely', () => {
toggle('foo')
expect(isFeatureEnabledMock).toHaveBeenCalledWith('foo', 'fooBSide', {
thisWillNotBeOverwritten: 'foo',
deviceType: 'mobile',
isLoggedIn: false
})

booleanToggle('foo')
expect(isFeatureEnabledMock).toHaveBeenCalledWith('foo', 'fooBSide', {
expect(optimizelyUserContextMock).toHaveBeenCalledWith('fooBSide', {
thisWillNotBeOverwritten: 'foo',
deviceType: 'mobile',
isLoggedIn: false
Expand All @@ -213,26 +200,23 @@ describe('Optimizely Integration', () => {
})

toggle('foo', 'a', 'b')
expect(isFeatureEnabledMock).toHaveBeenCalledWith('foo', 'fooBSide', {
expect(optimizelyUserContextMock).toHaveBeenCalledWith('fooBSide', {
valueAfterReset: true
})

toggle('foo')
expect(isFeatureEnabledMock).toHaveBeenCalledWith('foo', 'fooBSide', {
expect(optimizelyUserContextMock).toHaveBeenCalledWith('fooBSide', {
valueAfterReset: true
})
})
})

testAudienceSegmentationCacheBusting(toggle, activateMock)
testAudienceSegmentationCacheBusting(booleanToggle, isFeatureEnabledMock)

it("Returns Optimizely's value when no arguments supplied using booleanToggle", () => {
it("Returns Optimizely's value when no arguments supplied using", () => {
vlacerda marked this conversation as resolved.
Show resolved Hide resolved
// maps to a, b, c
expect(toggle('foo')).toEqual('b')
expect(toggle('bar')).toEqual('a')
expect(booleanToggle('foo')).toBeTruthy()
expect(booleanToggle('bar')).toBeFalsy()
})

it('Maps Optimizely value to a, b, c indexed arguments', () => {
Expand All @@ -257,27 +241,21 @@ describe('Optimizely Integration', () => {
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
expect(toggle('bar')).toEqual('a')
expect(toggle('baz')).toEqual('b')
expect(booleanToggle('bar')).toEqual(false)
expect(booleanToggle('baz')).toEqual(true)
})

it('allows you to invent non-existing experiments', () => {
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
expect(booleanToggle('baz')).toEqual(true)
})

it('persist after setAudienceSegmentationAttributes is called', () => {
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
setAudienceSegmentationAttributes({foo: 'bar'})
expect(toggle('foo', 'a', 'b', 'c')).toEqual('a')
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
expect(booleanToggle('bar')).toEqual(false)
expect(booleanToggle('baz')).toEqual(true)
})

it('makes sure Toggles return defaults if forced values are of wrong type', () => {
expect(toggle('baz', 'a', 'b', 'c')).toEqual('a')
expect(booleanToggle('bax')).toEqual(false)
})

describe('Clearing forced toggles', () => {
Expand All @@ -288,15 +266,11 @@ describe('Optimizely Integration', () => {
it('should yield real values for cleared toggles', () => {
expect(toggle('foo', 'a', 'b', 'c')).toEqual('b')
expect(toggle('bar', 'a', 'b', 'c')).toEqual('a')
expect(booleanToggle('foo')).toEqual(true)
expect(booleanToggle('bar')).toEqual(false)
})

it('should keep the non-cleared forced toggles and other defaults', () => {
expect(toggle('bax', 'a', 'b', 'c')).toEqual('c')
expect(booleanToggle('baz')).toEqual(true)
expect(toggle('nonexistent', 'a', 'b', 'c')).toEqual('a')
expect(booleanToggle('nonexistent')).toEqual(false)
})
})
})
Expand Down
Loading