-
Notifications
You must be signed in to change notification settings - Fork 37
fix: better error messages for js provider #635
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
base: main
Are you sure you want to change the base?
Conversation
4c70226
to
06b01ee
Compare
needs to be rebased |
6d15eee
to
2dcf40b
Compare
if (experiments && experimentgroups) { | ||
this.cachedExperiments = experiments; | ||
this.cachedExperimentGroups = experimentgroups; | ||
this.lastUpdated = new Date(); |
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.
Should this be date time?
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.
I don't think it exists
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.
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.
Do we need experiment group support in the provider? This is a resolution time thing right? Experiment bucketing should sort it out
What If someone needs direct experiment group information for debugging ? |
if (this.defaults) { | ||
console.log('Falling back to defaults'); | ||
if (isNetworkError(error)) { | ||
console.warn('SuperpositionProvider: Using fallback configuration - server connection failed'); |
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.
Why warn
? IMO this should be an error
log, less likely for people to miss it that way. Also, feel like this would be a better way to phrase the error: Server connection failed, using fallback configuration.
Basically, highlight the problem first.
2dcf40b
to
70ab56e
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change