feat: unset user properties when variant evaluates to none or is a fallback #13
Conversation
curtisliu
force-pushed
the
unset-properties
branch
from
0bb6e1e to
512c6a6
Compare
bgiori
left a comment
bgiori
left a comment
There was a problem hiding this comment.
First, thanks a lot for taking on this work. Im sure it can be frustrating to take a project started by someone else.
The fallback code is much cleaner now, thanks for doing that.
| public variant( | ||
| key: string, | ||
| fallback?: string | Variant, | ||
| trackExposure = true, |
There was a problem hiding this comment.
is there a common use case for this? I'd prefer not to add additional optional params to this function.
IMO a customer generally either wants to use exposure events or not? Not really a mix of both.
There was a problem hiding this comment.
Main reason for this is to allow them to call variant multiple times, then call it once when they actually want to send the event. Maybe an explicit track exposure call is the right interface, so I'll remove it from this PR
| export enum VariantSource { | ||
| LOCAL_STORAGE = 'storage', | ||
| INITIAL_VARIANTS = 'initial', | ||
| SECONDARY_LOCAL_STORAGE = 'secondary-storage', | ||
| SECONDARY_INITIAL_VARIANTS = 'secondary-initial', | ||
| FALLBACK_INLINE = 'fallback-inline', | ||
| FALLBACK_CONFIG = 'fallback-config', | ||
| } | ||
|
|
||
| export const isFallback = (source: VariantSource): boolean => { | ||
| return ( | ||
| source === VariantSource.FALLBACK_INLINE || | ||
| source === VariantSource.FALLBACK_CONFIG | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Not sure this belongs in this file.
Could put this and Source from config in its own file within /types
| */ | ||
| export interface ExperimentAnalyticsProvider { | ||
| track(event: ExperimentAnalyticsEvent): void; | ||
| unset(event: ExperimentAnalyticsEvent): void; |
There was a problem hiding this comment.
This is going to break the smart recruiters custom implementation. We should technically release a v2...
Maybe we discuss the change with them and work through their implementation together? I'd like to avoid publishing v2 until we overhaul the library. 😕
Alternatively, we could have the logic built into the event in track like we do with set user properties. and unset the property if the variant value is null (unassigned or fallback).
There was a problem hiding this comment.
I made this function optional, which should make it backwards compatible
| * Event for tracking a user's exposure to a variant. This event will not count | ||
| * towards your analytics event volume. | ||
| */ | ||
| export class ExposureEvent implements ExperimentAnalyticsEvent { |
There was a problem hiding this comment.
Is this pattern not common in js-land? In other words is it more "native" to use a function to return an interface implementation?
There was a problem hiding this comment.
for simple data objections, factory functions are preferable to classes
| expect(spyTrack).toBeCalledTimes(1); | ||
|
|
||
| expect(spyTrack).lastCalledWith({ | ||
| name: '[Experiment] Exposure', | ||
| properties: { | ||
| key: serverKey, | ||
| source: 'storage', | ||
| variant: serverVariant.value, | ||
| }, | ||
| user: expect.objectContaining({ | ||
| user_id: 'test_user', | ||
| }), | ||
| key: serverKey, | ||
| variant: serverVariant, | ||
| userProperties: { | ||
| [`[Experiment] ${serverKey}`]: serverVariant.value, | ||
| }, | ||
| userProperty: `[Experiment] ${serverKey}`, | ||
| }); | ||
|
|
||
| expect(spyUnset).toBeCalledTimes(0); |
There was a problem hiding this comment.
Ah this is cool, still such a noob with js testing frameworks.
| /** | ||
| * User properties to identify with the user prior to sending the event. | ||
| */ | ||
| userProperties?: Record<string, unknown>; |
There was a problem hiding this comment.
Since you added userProperty you dont need this anymore?
I am confused that we need both userProperty and userProperties.
There was a problem hiding this comment.
Yeah, this isn't necessary anymore but keeping it for backwards compatibility
|
@bgiori PTAL |
bgiori
left a comment
bgiori
left a comment
There was a problem hiding this comment.
A few changes need to made which have slipped through the cracks, but looks good otherwise 👍
| payload: {}, | ||
| } | ||
| }, | ||
| analyticsProvider: { |
packages/browser/src/types/client.ts
Outdated
| variant( | ||
| key: string, | ||
| fallback?: string | Variant, | ||
| trackExposure?: boolean, |
There was a problem hiding this comment.
This field was removed from the concrete implementation, but not from here.
Summary
This adds the ability to unset user properties when a variant has no value or is a fallback.
Other changes:
trackExposureparameter tovariant()to allow explicitly not tracking exposure. Defaults to true.Checklist
unsetmethod.