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

[WIP] add useTreatment hook #70

Draft
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

besh
Copy link

@besh besh commented Aug 31, 2021

React SDK

What did you accomplish?

Added a useTreatment hook that takes in a single split. Addresses #69

How do we test the changes introduced in this PR?

Added tests to verify the new hook. You can test locally via npm-link or yalc in a project repo. One test will fail until I modify further (hence WIP).

Extra Notes

Initially, I wanted to create a useTreatment hook that used client.getTreatment. However, the existing useTreatments hook is using getTreatmentsWithConfig under the hood, so I chose to follow the existing convention. Shouldn't it instead be using getTreatements and then have a useTreatementsWithConfig hook that uses getTreatmentsWithConfig? I understand that at this point, it would be a version breaking change.

The end API that I'd like is

const treatment = useTreatment('my_split');

if (treatment === 'on') {
  // do something
}

However, because I'm following the existing convention of the useTreatments hook, the resulting usage is instead

const treatmentResult = useTreatment('my_split');

if (treatmentResult.treatment === 'on') {
  // do something
}

Would you be okay if I added two hooks?

  • useTreatment -> uses client.getTreatment
  • useTreatmentWithConfig -> uses client.getTreatmentWithConfig

These additions wouldn't be version breaking as they are additive, but it wouldn't follow the existing convention set in place. Looking for feedback and thoughts here!

const useTreatment = (splitName: string, attributes?: SplitIO.Attributes, key?: SplitIO.SplitKey): SplitIO.TreatmentWithConfig => {
const client = checkHooks(ERROR_UT_NO_USECONTEXT) ? useClient(key) : null;
return client ?
client.getTreatmentWithConfig(splitName, attributes) :
Copy link
Author

Choose a reason for hiding this comment

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

Ideally, I'd use client.getTreatment here instead, then I'd also have a useTreatmentWithConfig hook that uses client.getTreatmentWithConfig. Are we okay with me making that addition?

const client = checkHooks(ERROR_UT_NO_USECONTEXT) ? useClient(key) : null;
return client ?
client.getTreatmentWithConfig(splitName, attributes) :
getControlTreatmentsWithConfig([splitName])[0];
Copy link
Author

Choose a reason for hiding this comment

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

I wanted to reuse the existing methods here, but I don't love passing in an array of a single item and returning the first entry.

I could expose validateSplit and create a new getControlTreatmentWithConfig that uses validateSplit instead of validateSplits.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant