Update Play to 2.8.2 (bp #2887) #2934
Conversation
ihostage
force-pushed
the
update_play_2_8_2_to_1_6_X
branch
from
4b6baa1
to
f9c1674
Compare
| @@ -552,7 +552,7 @@ lazy val `testkit-scaladsl` = (project in file("testkit/scaladsl")) | |||
| `persistence-core` % "compile;test->test", | |||
| `persistence-scaladsl` % "compile;test->test", | |||
| `persistence-cassandra-scaladsl` % "compile->test;test->test", | |||
| `persistence-jdbc-scaladsl` % Test | |||
| `persistence-jdbc-scaladsl` % "compile;test->test" | |||
There was a problem hiding this comment.
I just realised that this looks wrong: If I am a Cassandra user I shouldn't get persistence-jdbc-scaladsl as a transitive dependency.
Maybe it should be provided? But then there'd be a runtime error for cassandra users invoking ServiceTest since import com.lightbend.lagom.scaladsl.persistence.jdbc.SlickProviderComponents will raise a ClassNotFound.
There was a problem hiding this comment.
We discussed this approach offline and it sounded OK to me. But I'm not sure.
I guess it's the least bad of the options.
There was a problem hiding this comment.
Yes. I saw only 3 variants:
1)
Eagerly initialize slickProvider in com.lightbend.lagom.scaladsl.persistence.jdbc.SlickProviderComponents
=> lazy property with unlazy behavior 😞
Was discussed in #2887
2)
Add eager initialization in com.lightbend.lagom.scaladsl.testkit.ServiceTest#startServer
=> testkit-scaladsl depends on persistence-jdbc-scaladsl for compile scope 😞
(But…. testkit using only for tests and this artifact will not have in production classpath. And maybe it’s not a big problem for users if for tests will be loaded not used transitive dependency.)
3)
Try to implement a second approach by reflection
=> forget about beautiful source code 😂
There was a problem hiding this comment.
I think I'm beginning to lean towards Eagerly initialize slickProvider in com.lightbend.lagom.scaladsl.persistence.jdbc.SlickProviderComponents => lazy property with unlazy behavior 😞
| // We should eagerly init SlickProvider before test a JDBC persistence | ||
| if (setup.jdbc) Try(lagomApplication.asInstanceOf[SlickProviderComponents]).map(_.slickProvider) |
There was a problem hiding this comment.
what is the relation of this change with upgrading to Play v2.8.2?
Could we make it in another PR so we can track things better?
There was a problem hiding this comment.
Hmm, I see that this is a backport and it was on the original PR.
There was a problem hiding this comment.
Even if this is a backport, I had second thoughts (#2934 (comment)) wrt the impact this change has.
Now that's merged the question is: are we comfortable with the jdbc transitive dependency even for cassandra users? If not, we should roll this back and implement a different strategy. If yes, then we can move on to a 1.6.3 release.
No description provided.