-
Notifications
You must be signed in to change notification settings - Fork 21
fix(experimental): Add experimental LDScopedClient for incrementally building and propagating contexts #297
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
return c.client | ||
} | ||
|
||
// Contextual methods: equivalent to calling the same method on the underlying client with the current context |
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.
All of the remaining methods in this file mirror some existing method on LDClient
, but instead of taking a context
, use the c.CurrentContext()
.
All the doc comments are copied over mostly verbatim - I figured this would be better than just See LDClient.MethodName for more information
, because once a customer adopts the scoped client, there will be developers in that organization that only ever interact with the scoped client, and they may need quick explainers of each method if it's their first time using LD clients at all.
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.
Looking pretty good!
func (c *LDScopedClient) AddContext(contexts ...ldcontext.Context) { | ||
c.Lock() | ||
defer c.Unlock() | ||
c.rebuild = true |
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.
It is possible all the requested contexts could be dropped (due to conflicts), so you might be prematurely setting this to true. Minor optimization I realize.
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 think I'm going to keep this the way it is - the "suboptimal" thing will only happen if you're using this function wrong.
ldclient_scoped.go
Outdated
// The scoped client's contexts so far are combined into a multi-context whenever the | ||
// scoped client is used. |
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'm realizing that my mental model is that the scoped client has a single multi-context that it keeps adding to. However, the documentation here is emphasizing that a scoped client has a collection of contexts that are only combined into a multi-context when you call some other method on the client.
Is that difference an implementation detail, or a meaningful distinction that we should continue to clarify?
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.
Is that difference an implementation detail, or a meaningful distinction that we should continue to clarify?
@mmrj I'm still torn on this. Right now I think it's a meaningful distinction, because of the mutability. Contexts (and multi-contexts) are supposed to be immutable, so if I say the scoped client contains a multi-context, but also I'm "adding a context" to it, that could raise some questions. I would then clarify I'm not really mutating the multi-context, I'm creating a new multi-context (though not really, because this multi-context is imaginary). And I am mutating the scoped client.
I just updated the LDScopedClient documentation with this paragraph:
// A scoped client contains one or more contexts, each with a unique kind. You
// can add additional contexts with [LDScopedClient.AddContext], as long as the
// new contexts' kinds are not already present. The "current context" is the
// combination of all contexts added so far, as a multi-context.
Then I use the term "current context" whenever I mean the scoped client's current multi-context.
Does that make sense, or just make things more confusing?
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 makes sense to me! thanks for the updates to use "current context" more clearly
Co-authored-by: Matthew M. Keeler <[email protected]>
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.
✅ from Docs for the in-code documentation. I did not review the implementation or tests carefully.
// A scoped client contains one or more contexts, each with a unique kind. You | ||
// can add additional contexts with [LDScopedClient.AddContext], as long as the | ||
// new contexts' kinds are not already present. The "current context" is the | ||
// combination of all contexts added so far, as a multi-context. | ||
// | ||
// scopedClient := ld.NewScopedClient(client, userContext) | ||
// scopedClient.CurrentContext() // returns the single "user" context | ||
// scopedClient.AddContext(ldcontext.NewWithKind("company", "acme")) | ||
// scopedClient.CurrentContext() // returns a multi-context with a "user" and "company" context |
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 example is much clearer now, thanks
// scopedClient := ld.NewScopedClient(client, ldcontext.New("user-key")) | ||
// company := fetchCompanyForUser(user) | ||
// scopedClient.AddContext(ldcontext.NewWithKind("company", company.Id)) | ||
// scopedClient.BoolVariation("enterprise-features", false) |
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.
// scopedClient.BoolVariation("enterprise-features", false) | |
// scopedClient.BoolVariation("enterprise-features", false) // evaluates the flag using a multi-context with "user" and "company" contexts |
optional suggestion
ldclient_scoped.go
Outdated
// The scoped client's contexts so far are combined into a multi-context whenever the | ||
// scoped client is used. |
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 makes sense to me! thanks for the updates to use "current context" more clearly
🤖 I have created a release *beep* *boop* --- ## [7.13.2](v7.13.1...v7.13.2) (2025-08-14) ### Bug Fixes * **experimental:** Add experimental LDScopedClient for incrementally building and propagating contexts ([#297](#297)) ([2b96332](2b96332)) * **experimental:** Functions for putting/getting an LDScopedClient from context.Context ([#305](#305)) ([d61c744](d61c744)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
You can now create a "scoped client", a wrapper of the client that will use the specified context for all operations, including feature flag evaluations and event tracking. This facilitates propagating a context down to all logic that is related to a scoped piece of logic, like a web request.
An
LDScopedClient
implements most of the same methods asLDClient
, but without thecontext
argument.A scoped client is mutable, to facilitate incrementally building up a multi-context representing the current scope. You can add new contexts (as long as they aren't replacing an existing kind) with
LDScopedClient.AddContext
, and all contexts added so far will be combined into a multi-context whenever the scoped client is used.LDScopedClient.OverwriteContextByKind
is offered as an escape hatch if you need to update existing data.