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

vlacerda · 2024-06-05T15:10:40Z

https://app.shortcut.com/findhotel/story/119714/upgrade-optimizely-sdk-and-opticks-to-latest-version

Gerrit88

Thanks, Vini!

I left a few suggestions.

I also did some QA and everything seems to work as expected.

Gerrit88 · 2024-06-06T07:51:47Z

packages/lib/src/core/booleanToggle.test.ts

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.

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

README.md

Gerrit88 · 2024-06-06T08:31:15Z

packages/lib/src/integrations/optimizely.test.ts

  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'


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

Gerrit88 · 2024-06-06T08:39:05Z

packages/lib/src/integrations/optimizely.test.ts

@@ -26,7 +24,11 @@ import Optimizely, {
  // @ts-expect-error
  getEnabledFeaturesMock,


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

packages/lib/src/integrations/optimizely.test.ts

Gerrit88 · 2024-06-06T08:47:09Z

packages/lib/src/integrations/optimizely.ts

 ) =>
  optimizelyClient.notificationCenter.addNotificationListener(
-    NOTIFICATION_TYPES.DECISION,
+    OptimizelyLib.enums.NOTIFICATION_TYPES.DECISION,


With this change, we don't need the NOTIFICATION_TYPES declaration on line 19 anymore, right?

Gerrit88 · 2024-06-06T09:47:45Z

packages/lib/package.json

Remember to bump the version of the opticks package here.

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

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

luisfmsouza

I was trying to run this new version on the mobile app, but found something weird. Will comment here in case someoneelse also found and fixed it.

On the package/mobile-app/package.json file, I added the line below:

"opticks": "github:viodotcom/opticks#sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest",

After running the install command, this is how the opticks package folder looked like (without the code). Maybe this type of install is not supported?

How do you suggest I install this branch version into the app to test it out?

Gerrit88 · 2024-06-06T10:02:09Z

@luisfmsouza, I'm not sure the best way to run from GitHub.

But I QA'ed by pointing to the repo on my local machine:

"opticks": "../../../opticks/packages/lib",

Co-authored-by: Gerrit Burger <[email protected]>

vlacerda · 2024-06-06T11:25:39Z

I was trying to run this new version on the mobile app, but found something weird. Will comment here in case someoneelse also found and fixed it.

On the package/mobile-app/package.json file, I added the line below:
"opticks": "github:viodotcom/opticks#sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest",
After running the install command, this is how the opticks package folder looked like (without the code). Maybe this type of install is not supported?
How do you suggest I install this branch version into the app to test it out?

@luisfmsouza I tested with Opticks locally. The steps are roughly:

Clone opticks
Go to opticks/packages/lib folder and run yarn link
Go to the root of daedalus and run yarn link {../relative-path-to-opticks/packages/lib}
- This will add the link you added yourself but with the local version
On the webpack.config of the package you want to test you then need to include opticks to be compiled as well by babel.
In the property include here add path.resolve(__dirname, '../../../../opticks/packages/lib')

Gerrit88

Thanks for those changes, Vini!

It's probably best to do the fix I mentioned here in this PR as well.

jopdeklein · 2024-06-06T11:33:03Z

packages/lib/src/integrations/__mocks__/@optimizely/optimizely-sdk.ts

-    return [`${userId}-test-1`, `${userId}-test-2`]
-  }
+export const decideMock = jest.fn((toggleKey) => ({
+  enabled: toggleKey === "foo"


Suggested change

enabled: toggleKey === "foo"

enabled: toggleKey === 'foo'

jopdeklein · 2024-06-06T13:46:49Z

README.md

-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.
+We specify multiple variants of which one is active at any time.


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.

jopdeklein · 2024-06-06T13:48:01Z

README.md

-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.


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.

jopdeklein · 2024-06-06T13:56:18Z

packages/lib/src/integrations/optimizely.ts

-  )
-
-  return isInAnyAudience || !isEveryoneElseRulePaused
+  return isInAnyAudience


Since we're not explicitly checking the last fallback rule as "everyone else", does this construct rely on the fact we always have our own explicit "everyone else" audience rule?

It's supposed to stand on its own. If we don't have any audience rule set this means isInAnyAudience will be false. Which matches our expectations. If you don't have any rule set your experiment is always turned off.

Gerrit88

Thanks, Vini! 🚀

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

8c7dd43

vlacerda force-pushed the sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest branch from 3c7851c to 8c7dd43 Compare June 5, 2024 15:11

vlacerda requested review from dale-french, jopdeklein, luisfmsouza and Gerrit88 June 5, 2024 15:13

Updated README

38ab4b6

Gerrit88 requested changes Jun 6, 2024

View reviewed changes

luisfmsouza reviewed Jun 6, 2024

View reviewed changes

vlacerda and others added 3 commits June 6, 2024 12:38

Update README.md

4c2ff79

Co-authored-by: Gerrit Burger <[email protected]>

Apply suggestions from code review

28ff2e4

Co-authored-by: Gerrit Burger <[email protected]>

Code review

8c68cef

Code review

d750ed2

vlacerda force-pushed the sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest branch from f560af7 to d750ed2 Compare June 6, 2024 11:26

vlacerda requested a review from Gerrit88 June 6, 2024 11:32

Gerrit88 reviewed Jun 6, 2024

View reviewed changes

jopdeklein approved these changes Jun 6, 2024

View reviewed changes

Remove boolean toggle remnants

9b32a6a

vlacerda force-pushed the sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest branch from 927c403 to 9b32a6a Compare June 6, 2024 14:31

Code review

5e2c595

Gerrit88 approved these changes Jun 6, 2024

View reviewed changes

vlacerda merged commit d73f8eb into master Jun 7, 2024
1 check passed

vlacerda deleted the sc-119714-upgrade-optimizely-sdk-and-opticks-to-latest branch June 7, 2024 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vlacerda commented Jun 5, 2024 •

edited

Loading

Gerrit88 left a comment

Gerrit88 Jun 6, 2024

vlacerda Jun 6, 2024

Gerrit88 Jun 6, 2024

Gerrit88 Jun 6, 2024

Gerrit88 Jun 6, 2024

Gerrit88 Jun 6, 2024

dale-french Jun 6, 2024 •

edited

Loading

dale-french Jun 6, 2024

luisfmsouza left a comment

Gerrit88 commented Jun 6, 2024

vlacerda commented Jun 6, 2024

Gerrit88 left a comment

jopdeklein Jun 6, 2024

jopdeklein Jun 6, 2024

jopdeklein Jun 6, 2024

jopdeklein Jun 6, 2024

vlacerda Jun 6, 2024

Gerrit88 left a comment

		@@ -26,7 +24,11 @@ import Optimizely, {
		// @ts-expect-error
		getEnabledFeaturesMock,

	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.

	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.

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

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

Conversation

vlacerda commented Jun 5, 2024 • edited Loading

Gerrit88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dale-french Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luisfmsouza left a comment

Choose a reason for hiding this comment

Gerrit88 commented Jun 6, 2024

vlacerda commented Jun 6, 2024

Gerrit88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gerrit88 left a comment

Choose a reason for hiding this comment

vlacerda commented Jun 5, 2024 •

edited

Loading

dale-french Jun 6, 2024 •

edited

Loading