feat: add overide to get all impression events #121
Conversation
There was a problem hiding this comment.
I like the direction this is going in and think it makes a lot of sense 😄
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:
- Toggle is enabled and has impressionData enabled
- Toggle is enabled and does not have impression data enabled
- Toggle is disabled, so we don't know.
Would it be better to model it as boolean | "unknown" or something?
src/events-handler.ts
Outdated
| @@ -33,6 +35,7 @@ class EventsHandler { | |||
| context: IContext, | |||
| enabled: boolean, | |||
| featureName: string, | |||
| impressionData: boolean, | |||
There was a problem hiding this comment.
Would it make sense to use a name like impressionDataEnabled instead? It might make the name slightly clearer (seeing as it's a boolean and not actual impression data). Happy to be overruled here, though.
There was a problem hiding this comment.
jeah I think it would be better, but I just wanted top keep it the same as the featureToggle property, which currently is "impressionData".
Wdyt?
There was a problem hiding this comment.
I'm open to either. As long as we've considered the option, I'm happy for you to make a decision on it impressionDataEnabled or even impressionDataEnabledForFeature. It could cause some confusion when handling an impression data event that you also have a subproperty called impressionData, so I'm leaning towards being more explicit here.
| test('Should call getVariant event when impressionData is false and impressionDataAll is true', (done) => { | ||
| const bootstrap = [ | ||
| { | ||
| name: 'impression-variant', | ||
| enabled: true, | ||
| variant: { | ||
| name: 'disabled', | ||
| enabled: false, | ||
| }, | ||
| impressionData: false, | ||
| }, | ||
| ]; | ||
|
|
||
| const config: IConfig = { | ||
| url: 'http://localhost/test', | ||
| clientKey: '12', | ||
| appName: 'web', | ||
| bootstrap, | ||
| impressionDataAll: true, | ||
| }; | ||
| const client = new UnleashClient(config); | ||
| client.start(); | ||
|
|
||
| client.on(EVENTS.READY, () => { | ||
| const isEnabled = client.getVariant('impression-variant'); | ||
| expect(isEnabled).toBe(true); | ||
| }); |
There was a problem hiding this comment.
There's no test for whether impressionData events are returned when a toggle is false but impressionDataAll is true: is that supposed to have the same behavior?
There was a problem hiding this comment.
yes, I should add a test case for this as well. I did it for getVariant.
Co-authored-by: Thomas Heartman <thomas@getunleash.ai>
You make a strong argument here. We could make the |
|
@ivarconr Yeah, I think using |
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. |
|
Yeah, alright, I can buy that 😄 |
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: