stripe-scala icon indicating copy to clipboard operation
stripe-scala copied to clipboard

Investigate DI (use case classes instead of objects?)

Open mdedetrich opened this issue 8 years ago • 1 comments
trafficstars

For basic top level models (i.e. org.mdedetrich.stripe.v1.Accounts), these are currently represented as objects. While very convenient, it makes DI harder because we have to mention the dependencies in every internal method of the Object, i.e.

def get(id: String)(implicit apiKey: ApiKey, endpoint: Endpoint): Future[Try[Account]] = {
    val finalUrl = endpoint.url + s"/v1/accounts/$id"

    createRequestGET[Account](finalUrl, logger)
  }

We could kinda get away with this (for now) because

  • We used a global dispatch singleton Http client instance (the current version of stripe-scala doesn't actually let you specify a httpClient object if you don't want to use the global one)
  • There aren't that many DI dependencies for it to become painful

However with the move to akka-http (and possibly some other changes that will happen in the future), it may make some sense to use a saner mechanism. What I have immediately in mind (without having to resort to other external dependencies) is instead of using objects we use implicit final case classes, i.e. instead of

object Accounts extends LazyLogging

we have

final case class Accounts(implicit httpClient: HttpExt, executionContext: ExecutionContext....) extends LazyLogging

This however means that instead of doing Accounts.update(....) we would have to do Accounts().update(....). We also create an instance of Accounts() whenever its called, I think the JVM is good at handling this stuff but not entirely sure

@leonardehrenfried Do you have any ideas. We could just do what we do currently and just add all of the dependencies onto the internal object functions

There are also other options, i.e. using stuff like MacWire or Subcut for DI.

mdedetrich avatar Feb 04 '17 22:02 mdedetrich

I think that using a global singleton for a the HTTP client isn't too bad so I'm not sure if the configurability is needed at this point. I have seen few client libraries that let you inject your own http client and I never really had the desire to swap out the HTTP implementation. It also keeps it easier to use the library if you don't have to instantiate a httpClient yourself.

Personally I would get rid of the repetition in the method signature by defining traits for the different HTTP methods to which the implementer only would have to supply the data that is actually different per entity.

A simplified example:

trait HttpGet[A] {
  def getUrl(id:String): String

  def get[A](id: String)(implicit apiKey: ApiKey, endpoint: Endpoint): Future[Try[A]] = {
    val finalUrl = endpoint.url + getUrl(id)
    createRequestGET[A](finalUrl, logger)
  } 
}

object Accounts extends HttpGet[Account]{
  def getUrl(id:String): String = s"/v1/accounts/$id"
}

Thinking about it, those two aren't mutually exclusive. You can still pass the httpClient to the method that way if you don't want to use the global one.

Something that also came to me is to supply a default implementation just like ExecutionContext.global does. We can also add an error message that makes it easy to discover this.

leonardehrenfried avatar Feb 07 '17 12:02 leonardehrenfried