feat: buildAndInit for isolated client instances#166
Open
Conversation
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
5c466e9 to
a73bf0c
Compare
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Jan 31, 2025
typotter
commented
Feb 14, 2025
Collaborator
Author
|
Several false starts and iterations later, we're ready for another review. Note: this PR is now stacked on #170 where changes are made just to how the singleton is referenced, leaving the focus of this PR on |
aarsilv
reviewed
Feb 16, 2025
felipecsl
reviewed
Feb 18, 2025
Contributor
felipecsl
left a comment
There was a problem hiding this comment.
Main feedback is around documentation and providing alternative APIs for what's been deprecated. I'm ready to approve after that, great work!
Collaborator
Author
|
Thank you, @rasendubi @felipecsl @aarsilv for you reviews and patience as we explore this change. I have made a key change - the singleton PTAL |
felipecsl
approved these changes
Feb 21, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
labels: mergeable
Eppo Internal
🎟️ Fixes: FF-3888
📜 Multiple Eppo Clients
Configuration Side-loading
Motivation and Context
The
initandgetInstance()methods work on a singleton instance of theEppoClientwhich makes for a generally smooth developer experience given that the Eppo SDK essentially handles dependency management for the dev. This does have its own drawbacks, but those are not particularly relevant here.There are use cases, however, when a non-singleton instance of the
EppoClientis required. One such use case is embedding the Eppo SDK into a library which itself can be included in other applications. If that 3p library shipped with Eppo is integrated into another application that has the Eppo SDK running, there would be undefined behaviour as the SDK cannot handle this use case.The
initmethod (and class constructors) forEppoClientand subclasses have evolved organically over time, adding new options as new features are added to the clients. The very large options type is beginning to become a little untenable and disorganized so we take the opportunity to clean that up a bit here.There are other limitations and drawbacks to the current model of instantiating an
EppoClientstatically and then initializing it when the code callsinitincluding the awkward coupling of needing to wait for init to resolve in order to get a reference to an initialized client. We have an opportunity to decouple initialization and waiting to make for a better DX (in addition to giving the dev full control over managing theEppoClientreference (intrinsically allows for easier mocking in tests and will be consistent with the host applications existing DI approach).This change must be done without a major version bump and completely preserve the existing singleton API
Description
IClientConfigname (this keeps the change backwards compatible while offering a big win in option clarity)- New method:
buildAndInitwhich creates an isolated client instance.- new method:
waitForConfigurationwhich resolves when the first config is loaded.How has this been documented?
How has this been tested?