Added static context fields #44 #53
Conversation
There was a problem hiding this comment.
@wesleimp Thanks for your contribution! I think the patch is missing a key piece of functionality though. The static context fields are never actually passed down to the strategy here so I don't think this will work as intended.
The IsEnabled function can take an optional WithContext() feature option which specifies the context, but if the user does not specify the context, it should fallback to using the static context fields as the default. So I think what is missing here is to extend the IsEnabled function such that the static context is passed down to the strategy. You can draw inspiration from the Node.js implementation in this commit, but unfortunately Go has no equivalent of Object.assign so we might need to add a Merge function to the Context struct or something. If you add a Merge function, please add a unit test as well. I'm happy to discuss other solutions here too :)
Let me know if you need more help finishing this or if you would prefer someone else from the community to take it over.
@jrbarron Thanks for your comment. I got it. I'm going to review my implementation and make some changes. If you prefer, we can discuss about it on slack. Sorry for my inattention, I'll fix it. |
| @@ -243,6 +250,11 @@ func (uc Client) IsEnabled(feature string, options ...FeatureOption) (enabled bo | |||
| // TODO: warnOnce missingStrategy | |||
| continue | |||
| } | |||
|
|
|||
| staticContext := &context.Context{AppName: uc.options.appName, Environment: "default"} | |||
There was a problem hiding this comment.
We shouldn't need to create this every time because the "static" part of this will never change once the client has been initialized. Maybe a better solution would be that the Client contains a staticContext field which gets initialized only once?
|
|
||
| staticContext := &context.Context{AppName: uc.options.appName, Environment: "default"} | ||
|
|
||
| mergo.Merge(&opts.ctx, staticContext, mergo.WithOverride) |
There was a problem hiding this comment.
I personally feel that it is a bit overkill to import a new package just to get this functionality. Mergo uses reflection internally to see which fields need to be merged and this has a runtime performance penalty. Since we are only working with a single struct of a well known type I think we can just implement our own function for this so we have full control of how contexts are merged now and in the future.
I started on this issue a few weeks ago and have an implementation ready (with a unit test) if you just want to take it, otherwise feel free to take a stab at it yourself ;)
There was a problem hiding this comment.
@wesleimp Great progress, but I think there are a few things we can improve here performance wise. Also I think the PR should target the v3 branch of the client since that is the one with Go module support and support for the v3 server.
|
@wesleimp We are planning on merging in the FlexibleRolloutStrategy and Constraints stuff this week which requires the static context fields. How would you like to proceed with this PR? Do you think you have time to make the suggested changes? Or should I just take this PR "as is" and add some patches on top of it to finish it off? |
@jrbarron you can add some patches on top of it. My time is so limited in these days and I can't take it over for now. |
|
Closing this PR since the relevant commits have been cherry-picked into #56 |
No description provided.