-
Notifications
You must be signed in to change notification settings - Fork 12
feat: unset user properties when variant evaluates to none or is a fallback #13
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
Changes from all commits
512c6a6
fef5510
db4957d
40bc846
685dcd8
53ead20
933869a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { VariantSource } from './source'; | ||
| import { ExperimentUser } from './user'; | ||
| import { Variant } from './variant'; | ||
|
|
||
|
|
@@ -19,48 +20,74 @@ export interface ExperimentAnalyticsEvent { | |
| * Event properties for the analytics event. Should be passed as the event | ||
| * properties to the analytics implementation provided by the | ||
| * {@link ExperimentAnalyticsProvider}. | ||
| * This is equivalent to | ||
| * ``` | ||
| * { | ||
| * "key": key, | ||
| * "variant": variant, | ||
| * } | ||
| * ``` | ||
| */ | ||
| properties: Record<string, string>; | ||
|
|
||
| /** | ||
| * User properties to identify with the user prior to sending the event. | ||
| * This is equivalent to | ||
| * ``` | ||
| * { | ||
| * [userProperty]: variant | ||
| * } | ||
| * ``` | ||
| */ | ||
| userProperties?: Record<string, unknown>; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since you added I am confused that we need both userProperty and userProperties.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, this isn't necessary anymore but keeping it for backwards compatibility |
||
| } | ||
|
|
||
| /** | ||
| * 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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this pattern not common in js-land? In other words is it more "native" to use a function to return an interface implementation?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for simple data objections, factory functions are preferable to classes |
||
| name = '[Experiment] Exposure'; | ||
| properties: Record<string, string>; | ||
| userProperties?: Record<string, unknown>; | ||
|
|
||
| /** | ||
| * The user exposed to the flag/experiment variant. | ||
| */ | ||
| public user: ExperimentUser; | ||
| user: ExperimentUser; | ||
|
|
||
| /** | ||
| * The key of the flag/experiment that the user has been exposed to. | ||
| */ | ||
| public key: string; | ||
| key: string; | ||
|
|
||
| /** | ||
| * The variant of the flag/experiment that the user has been exposed to. | ||
| */ | ||
| public variant: Variant; | ||
| variant: Variant; | ||
|
|
||
| public constructor(user: ExperimentUser, key: string, variant: Variant) { | ||
| this.key = key; | ||
| this.variant = variant; | ||
| this.properties = { | ||
| key: key, | ||
| variant: variant.value, | ||
| }; | ||
| this.userProperties = { | ||
| [`[Experiment] ${key}`]: variant.value, | ||
| }; | ||
| } | ||
| /** | ||
| * The user property for the flag/experiment (auto-generated from the key) | ||
| */ | ||
| userProperty: string; | ||
| } | ||
|
|
||
| /** | ||
| * Event for tracking a user's exposure to a variant. This event will not count | ||
| * towards your analytics event volume. | ||
| */ | ||
| export const exposureEvent = ( | ||
| user: ExperimentUser, | ||
| key: string, | ||
| variant: Variant, | ||
| source: VariantSource, | ||
| ): ExperimentAnalyticsEvent => { | ||
| const name = '[Experiment] Exposure'; | ||
| const value = variant?.value; | ||
| const userProperty = `[Experiment] ${key}`; | ||
| return { | ||
| name, | ||
| user, | ||
| key, | ||
| variant, | ||
| userProperty, | ||
| properties: { | ||
| key, | ||
| variant: value, | ||
| source, | ||
| }, | ||
| userProperties: { | ||
| [userProperty]: value, | ||
| }, | ||
| }; | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be updated