statsig-io / react-sdk

An SDK for using Statsig Feature Management and Experimentation platform in React js clients
ISC License
6 stars 6 forks source link

Add exposureLoggingDisabled Argument to useConfig, useExperiment, useGate, and useLayer Hooks #15

Closed anthonybarsotti closed 10 months ago

anthonybarsotti commented 10 months ago

For the useConfig, useExperiment, useGate, and useLayer hooks, it would be ideal if these allowed you to set the exposureLoggingDisabled value via an additional argument. We currently have to conditionally choose whether or not to use useExperiment or useExperimentWithExposureLoggingDisabled based on our users' cookie policy consent which looks something like this:

const userConsented: boolean = getUserConsent();
const useExperimentWithUserConsentPreference = userConsented
    ? useExperiment
    : useExperimentWithExposureLoggingDisabled;
// Have to type this as any to satisfy TS
const options: any = userConsented ? true : { keepDeviceValue: true };
const { config } = useExperimentWithUserConsentPreference(
  experimentName,
  options,
);

It would be ideal if we could just do this instead:

const userConsented: boolean = getUserConsent();
const { config } = useExperiment(
  experimentName,
  true, // keepDeviceValue
  false, // ignoreOverrides
  !userConsented, // Disable exposure logging for users who have denied tracking
);
tore-statsig commented 10 months ago

Those hooks should all be supported already https://github.com/statsig-io/react-sdk/blob/main/src/index.ts#L35

tore-statsig commented 10 months ago

Oh I see, I misread - you are aware of the exposure logging disabled hooks, you just wish it was a parameter. We will consider that for a future breaking change, but as is, you can easily write a wrapper component yourself that takes in the parameters you want and decides which method to call

anthonybarsotti commented 10 months ago

I don't think it would be a breaking change, that new argument would be optional and set to false by default so existing implementations wouldn't be impacted. If you want I could open a PR with what I have in mind. The issue with the wrapper component is that it would effectively need to be three different wrapper components:

  1. A wrapper component that calls useExperiment internally
  2. A wrapper component that calls useExperimentWithExposureLoggingDisabled internally
  3. A wrapper component that conditionally renders the other two wrapper components based on the user's consent state

Without that you'd either have to call the hooks conditionally which breaks the rule of hooks needing to be called deterministically or you'd have to do something like we're currently doing.