search

statsig-io / java-server-sdk

An SDK for Java or Kotlin intended for multi-user/server environments
ISC License
7 stars 3 forks source link

StatsigServer Interface and Singleton Class #3

Closed pablobaxter closed 2 years ago

pablobaxter commented 2 years ago

This is what I meant by having StatsigServer be an interface, which allows for multiple different "server" connections, each self contained, and only accessible from one class.

In Kotlin, the call would look like this:

val server = StatsigServer("key", StatsigOptions())

and in Java it'll be:

StatsigServer server = Statsig.createServer("key", new StatsigOptions())

The name for Java can be something else (it's as simple as renaming the @JvmName() string).

With this change, calling shutdown() essentially destroys the StatsigServer, with any new calls to this object throwing an exception. The client would need to create a new StatsigServer to be able to utilize the connection again.

tore-statsig commented 2 years ago

Yeah I think overall this is an improvement - we started out with Statsig only being accessible as a singleton, and then tacked on the ability to use multiple instances. We should definitely have a shared interface between the two, but I still think we want to provide Statsig as a singleton to make it easy on folks (this should be simple to add), and we will likely continue to suggest people to use Statsig singleton for simplicity unless they truly have a multi-project setup on their backend.

pablobaxter commented 2 years ago

Yeah I think overall this is an improvement - we started out with Statsig only being accessible as a singleton, and then tacked on the ability to use multiple instances. We should definitely have a shared interface between the two, but I still think we want to provide Statsig as a singleton to make it easy on folks (this should be simple to add), and we will likely continue to suggest people to use Statsig singleton for simplicity unless they truly have a multi-project setup on their backend.

That makes sense, and it should be easy to implement.

tore-statsig commented 2 years ago

Feel free to mark this as ready for review, or I will soon and merge it :)

Verified the ServerSDKConsistencyTest, so we are good to go whenever

tore-statsig commented 2 years ago

Alright, looking good. Thanks for the revisions! I'm going to merge this for now, and then noodle a bit on the implications of this breaking change. The API hasn't changed drastically, and I do like this more for the long term.