buildo / nozzle

MIT License
0 stars 1 forks source link

DI macro/annotations-free injection #10

Closed calippo closed 8 years ago

calippo commented 8 years ago

In general I like the new DI approach. However, I think we can obtain a very similar result using less powerful language features.

Motivations

Considering this code

implicit val logging = nozzle.logging.BasicLogging()

import nozzle.config._
implicit val configProvider =
      ConfigProvider.empty
        .add(CampingControllerConfig("Le Marze"))

implicit val campingController = CampingControllerImpl.apply
implicit val campingRouter = CampingRouterImpl.apply

I have a couple of problems with this: 1) we are using implicits too much 2) in CampingControllerImpl.apply it's not clear to me what argument are applying to what function [(╯‵Д′)╯彡┻━┻ - angry guy flipping a table]

1) I would use implicits only when passing the same module to different other modules a dozen of times (cit. Haoyi). For instance, I would rewrite the code as follows:

implicit val logging = nozzle.logging.BasicLogging()

import nozzle.config._
implicit val configProvider =
      ConfigProvider.empty
        .add(CampingControllerConfig("Le Marze"))

val campingController = CampingControllerImpl.apply
val campingRouter = CampingRouterImpl.apply(campingController)

since campingController is used at most 2/3 times there's no need to make it implicit. I don't see drawbacks in using constructor injection instead of creating another implicit parameter in this case. I think it's just an improvement (the code is definitely clearer.)

2) Although I see why you called that method apply, I think the term is quiet misleading. It's not immediately clear to me what's going on here... you're actually reusing a common scala term, with a specific semantic, to do DI (or better, you are just trying to avoid to write implicit in the module). I would replace apply with something like applyAndInject or inject. It could be trivially done changing:

val compDecl = compDeclOpt.getOrElse(q"""
  object ${className.toTermName} {
    def apply(implicit ..$fields): $className = new $className(..$args)
  }
"""

to

val compDecl = compDeclOpt.getOrElse(q"""
  object ${className.toTermName} {
    def inject(implicit ..$fields): $className = new $className(..$args)
  }
""")

DI macro/annotations-free injection

That said, what about doing DI with constructor injection? When can use implicit parameters when we really need them (e.g. logging)

Without using annotations and macros:

  implicit val logging = nozzle.logging.BasicLogging()

  import nozzle.config._
  implicit val configProvider =
      ConfigProvider.empty
        .add(CampingControllerConfig("Le Marze"))

  val campingController = new CampingControllerImpl
  val campingRouter = new CampingRouterImpl(campingController)

  val server = Server(
    "test",
    ServerConfig("0.0.0.0", 8085),
    { implicit actorRefFactory =>
      campingRouter.route
    })

There's just a little overhead when defining a module, i.e. you need to separate parameters from implicit parameters.

class CampingRouterImpl(campingController: CampingController)
(implicit val logger: ModuleLogger[CampingController]) extends CampingRouter {

PROS:

CONS:

@utaal @gabro @danielegallingani thoughts? PS: I will move this to an issue, it probably makes more sense

utaal commented 8 years ago

You correctly point out that the macro annotation is likely unnecessary, even if we want to retain the ability to perform full injection with implicits. I guess I convinced myself I couldn't just annotate the class parameters themselves with implicit and needed the macro for that, but if we can do without it that's 10 fewer lines to maintain and one less feature to document. The only downside is we lose the ability to customize the error message when a module instance is not available in scope (we could have annotated all applys (or injects) with @implicitNotFound; the macro is probably not worth it here anyway.

That said, what about doing DI with constructor injection?

Sounds good.

There's just a little overhead when defining a module.

Looks completely acceptable to me.

My only concern is how verbose this gets when employed in larger apps: we'll see how it turns out when we convert one of the internal apps to nozzle-1.0.

I don't see the need to move this to an issue, just add a commit that removes the module macro and I'd be ok to merge (will wait for @gabro @danielegallingani to chime in if they like).

gabro commented 8 years ago

Agree with the approach. Let's fall back to macros if this gets cumbersome, but always keeping them opt-in. Big :+1:

calippo commented 8 years ago

My only concern is how verbose this gets when employed in larger apps

We absolutely need to test it in a large project... Fortunately, using scala, we don't fear refactoring 彡゚◉ω◉ )つー☆*