Seperate out service impl and Play router

[richdougherty]

Current code generation creates an abstract class that is both a Play router and a service class. This makes it easier to wire together the application (no need to configure a binding for the service implementation) but it's not the usual way that Play apps are written. Usually the router is one class and the controller (with action logic) is another. I think it would make sense if we followed this pattern for gRPC code generation too - have the router as one class and the service implementation as another.

Example code snippets taken from codegen as at 21 August 2018:

abstract class AbstractGreeterServiceRouter(mat: Materializer) extends PlayRouter(mat, GreeterService.name) with GreeterService {
  final override def createHandler(serviceName: String, mat: Materializer): HttpRequest => Future[HttpResponse] =
    GreeterServiceHandler(this, serviceName)(mat)
}

@Singleton
class GreeterServiceImpl @Inject() (implicit mat: Materializer) extends AbstractGreeterServiceRouter(mat) {
  override def sayHello(in: HelloRequest): Future[HelloReply] = Future.successful(HelloReply(s"Hello, ${in.name}!"))
}

[raboof]

The trade-off here is between having the router separate from the service class on the one hand, and requiring the user to add extra binding logic on the other hand.

I agree having the router separate would be great - the question is whether it's worth confronting the user with the extra complexity of manually binding so early... especially since when the binding is missing the error message might not be so easy to understand for a new user. Is there something we can do to have both?

[dwijnand]

Looks like this issue is interested in reverting part of #291, so /cc @johanandren too.

I think this thread https://typesafe.slack.com/archives/C26GD21T3/p1534317332000100 and the threads/issues/PRs it links to are possibly also relevant?

Looks like a hot topic, I'll find something easier.. 🙂

[johanandren]

I really think this should only be done if we can think of a scenario where a separate router would provide any concrete value to the user.

Given how the gRPC-server routes work with a constant path that cannot be changed and providing multiple actual routes with one entry I don't think the familiarity card is good enough motivation for re-introducing the boilerplate.

[ignasi35]

I think the changes in #378 open an interesting discussion specially to avoid userland code to extand both PlayRouter and the service interface from a single class. I like the the idea of composition #378 introduces.

At same time, the change suggested in #378 requires more work from the user in Play. I'd rather not merge #378 yet and move forward with the API as is (user code extends an abstract PlayRouter that implements the gRPC service interface) and see if there are any alternatives.

[dwijnand]

See #378 (comment).