Skip to content
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

Deprecate FlagsmithClient constructors with more than 1 parameter #135

Open
rolodato opened this issue Jan 21, 2025 · 1 comment · May be fixed by #136
Open

Deprecate FlagsmithClient constructors with more than 1 parameter #135

rolodato opened this issue Jan 21, 2025 · 1 comment · May be fixed by #136

Comments

@rolodato
Copy link
Member

I want to rename EnableClientSideEvaluation to EnableLocalEvaluation for consistency with every other Flagsmith SDK. This is currently impossible without introducing breaking changes:

The FlagsmithClient constructor has explicit ordered parameters. We can only add optional parameters at the end, and not rename any existing parameters to avoid breaking changes. This constructor is the main one used in our docs. Customers might be using named parameters, but nothing prevents them from using ordered parameters like we do in our own constructor which exclusively uses ordered parameters.

We do have another constructor with a dedicated configuration object which also has a second HttpClient parameter, for some reason I don't understand, instead of being incorporated into the FlagsmithConfiguration object. Using this constructor is slightly less ergonomic than the previous one, but gives us more flexibility to manipulate the public API for initialising clients:

new FlagsmithClient("ser.my-api-key", enableClientSideEvaluation: true, ...)
// vs
new FlagsmithClient(new FlagsmithConfiguration
{
    EnvironmentKey = "ser.my-api-key"
});

This constructor is currently only referenced in the "Singleton Initialisation" section on the same docs.

To avoid duplicating parameter documentation and having redundant constructors, we should introduce a new Flagsmith(IFlagsmithConfiguration) constructor, and deprecate all others. Later we can add a Builder pattern to construct clients, which will be backwards compatible with the new single-argument constructor, and is slightly less verbose than manually instantiating a FlagsmithConfiguration object.

@rolodato
Copy link
Member Author

Planning on deprecating IFlagsmithConfiguration as part of this change as well. It's purely a data object with no methods, except for an IsValid method that is not called anywhere, which has no need for an interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant