Unleash / unleash-proxy-client-js

A browser client that can be used together with the unleash-proxy.
Apache License 2.0
44 stars 46 forks source link

feat: add overide to get all impression events #121

Closed ivarconr closed 1 year ago

ivarconr commented 1 year ago

Make it possible to enable impression data for all isEnabled / getVariant calls. This PR also introduces a "impressionData" flag to the event itself, so the implementer can choose to discard events when we do not know whether impressionData is enabled for this toggle.

The reason we need this override in the proxy-sdk is that it simply does not know anything about disabled toggles (by design, to not leak information to the end-user about toggles). This makes it impossible to get impression data for disabled toggles, as they are not exposed to the frontend at all.

Todo:

ivarconr commented 1 year ago

I like the direction this is going in and think it makes a lot of sense smile

I wonder whether reducing impressionData to a boolean is a good idea, though. It's actually one of three states, so using a boolean, you lose information:

  1. Toggle is enabled and has impressionData enabled
  2. Toggle is enabled and does not have impression data enabled
  3. Toggle is disabled, so we don't know.

Would it be better to model it as boolean | "unknown" or something?

You make a strong argument here. We could make the impressionData property optional and only include it when the toggle definition exists. This would work in js environments anyway as we have the undefined as falsy behavior.

thomasheartman commented 1 year ago

@ivarconr Yeah, I think using boolean | undefined would work fine too. That does cover the use cases, though it makes it possible to "forget" to add it. When we make it required, you have to provide a value, which may prevent hard-to-spot bugs in the future 💁🏼

ivarconr commented 1 year ago

boolean | "unknown"

this will only work if the user of this SDK also uses typescript. The only alternative is to use string values, which feels a bit verbose in this use case. I think the only reason to enable this flag is that you really want impression events for all, and optionally have your own whitelist for what to actually store.

thomasheartman commented 1 year ago

Yeah, alright, I can buy that 😄