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

Remove auto module for ClassicActorSystemProvider #309

Merged
merged 3 commits into from
Nov 23, 2020

Conversation

octonato
Copy link
Contributor

@octonato octonato commented Oct 29, 2020

This is not compatible with Play 2.8.3 as Play has already a binding for ClassicActorSystemProvider.

When enabled we get

A binding to akka.actor.ClassicActorSystemProvider was already configured at play.grpc.ClassicActorsystemProviderModule.<init>(ClassicActorsystemProviderModule.scala:13):
[error] Binding(interface akka.actor.ClassicActorSystemProvider to ProviderConstructionTarget(class play.api.libs.concurrent.ActorSystemProvider)) (via modules: com.google.inject.util.Modules$OverrideModule -> play.api.inject.guice.GuiceableModuleConversions$$anon$4).

Still in draft, needs to adapt the docs.

I suggest we document that Play 2.8.3 users need to disable the module for now.

play.modules.disabled += "play.grpc.ClassicActorsystemProviderModule"

And as soon we cut a new Play gRPC we document the other way around.

References #317

@raboof
Copy link
Member

raboof commented Oct 29, 2020

Could we delete play.grpc.ClassicActorsystemProviderModule and require Play 2.8.3?

@ignasi35
Copy link
Member

@octonato I've reviewed this after our call and I think I agree with @raboof and prefer deleting the module.

Users of Play 2.8.2 or prior bumping play-grpc won't get a ClassicActorSystemProvider from either the Play or the play-grpc bindings but it's a oneliner they can add themselves. I think we can safely document the alternatives and delete the play-grpc module altogether.

@ignasi35
Copy link
Member

Tests fail because the module is missing, maybe we can already require 2.8.3 in this PR and solve it all at once.

@octonato
Copy link
Contributor Author

I agree in removing it. I will take that in consideration when moving this draft to ready for review.

@octonato
Copy link
Contributor Author

@ignasi35, we may want to wait for 2.8.4 before we cut a new release here.

@octonato octonato force-pushed the rgc/dont-load-classic-sys-module branch from 2ad9d31 to 3891c92 Compare October 30, 2020 19:33
@octonato octonato marked this pull request as ready for review October 30, 2020 19:34
@octonato
Copy link
Contributor Author

I did the updates as agreed, but without the documentation about how to create the module.

I haven't done is because it's not a one-liner, as we said. There is Java and Scala and also Lagom users. So, maybe we can discuss this strategy a little more.

Lagom users are bound to a fix Play version which is not 2.8.3 at the moment. Maybe the easiest is to keep the class and only document how to enable the module.

Anyway, we are not in a hurry. Let's give more time to think and consider the options.

@ignasi35
Copy link
Member

ignasi35 commented Nov 2, 2020

This is related to #312

@raboof
Copy link
Member

raboof commented Nov 23, 2020

I did the updates as agreed, but without the documentation about how to create the module. I haven't done is because it's not a one-liner, as we said

Perhaps we should just document "this version of play-grpc requires at least play 2.8.3, so if you want an older version of play you need to stick to an older version of play-grpc"?

@ignasi35 ignasi35 merged commit 2387585 into master Nov 23, 2020
@ignasi35 ignasi35 deleted the rgc/dont-load-classic-sys-module branch November 23, 2020 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants