-
Notifications
You must be signed in to change notification settings - Fork 83
feat: Implement createUserContext for decide API #632
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
Conversation
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.
UserContext looks good.
A couple of changes suggested.
} | ||
|
||
getAttributes(): UserAttributes { | ||
return this.attributes; |
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.
Can we return a copy of attributes?
The attributes can be changed by setAttribute while decide using it.
}) { | ||
this.optimizely = optimizely; | ||
this.userId = userId; | ||
this.attributes = attributes ?? {}; |
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.
Can we keep a copy of the given attributes here, so that any changes in the caller attributes after creating userContext are not reflected.
}); | ||
}); | ||
}); | ||
}); |
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.
Can we have one more test validating that changing caller attributes after createUserContext is not reflected in the create userContext?
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.
LGTM!
@@ -189,6 +189,7 @@ export const DECISION_NOTIFICATION_TYPES = { | |||
FEATURE_TEST: 'feature-test', | |||
FEATURE_VARIABLE: 'feature-variable', | |||
ALL_FEATURE_VARIABLES: 'all-feature-variables', | |||
FLAG: 'flag', |
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.
Does this need to be added now, or a later PR?
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.
In the next pr. Will remove it from here. Thanks!
5114c19
to
d83c3ec
Compare
d83c3ec
to
e4f595c
Compare
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.
LGTM
Summary
createUserContext
in optimizely clientOptimizelyUserContext
class and implementsetAttribute
methoddecideConfig
for future decide API testingTest
Issues