Java support for routes compiler#13213
Conversation
6c60e32 to
ef7f4d0
Compare
There was a problem hiding this comment.
I will implement adoption to this feature for Gradle Plugin in the separated PR
There was a problem hiding this comment.
OK. So in your next PR you will just make use of the new generated java routes for the gradle plugin or did you plan to also enable it for the sbt java plugin?
There was a problem hiding this comment.
If you want you can also already submit the next PR (including the commit of this PR). Just so I can take a look.
I can still review them separate and merge them separate.
There was a problem hiding this comment.
So in your next PR you will just make use of the new generated java routes for the gradle plugin or did you plan to also enable it for the sbt java plugin?
Only for Gradle plugin. Yes, I thought about enable a java routes by default for PlayJava sbt plugin, but decided don't do that before we'll get a feedback from someone who enable it manually. If you think we are ready do that right now, I will be happy do that.
| methodPart + paramPart | ||
| } | ||
|
|
||
| def javaInjectedControllerMethodCall(r: Route, ident: String): String = { |
There was a problem hiding this comment.
A lot of changes in this file can be combine and optimise for both languages. But I decided keep it to the future yet and avoid touching an exists functions for Scala and wrote a "duplicate" for Java.
@mkurz if you want, I can try to do this unification in this PR.
ef7f4d0 to
1cd6053
Compare
|
Will take a look in the next 24 hours |
| (packageName.map(_.replace(".", "/") + "/").getOrElse("") + JavaWrapperFile) -> { | ||
| lang match { | ||
| case Language.JAVA => | ||
| static.twirl.javaJavaWrappers(sourceInfo, namespace, packageName, controllers, jsReverseRouter).body |
There was a problem hiding this comment.
Yep. It looks not good, but It's just a java prefix for all existed templates.
There was a problem hiding this comment.
Hmm.. yes it's a bit confusing to read.
Just thinking out loud, maybe we should also rename existing template files:
jJavascriptReverseRouter.scala.twirl
jJavaWrappers.scala.twirl
jReverseRouter.scala.twirl
jRoutesPrefix.scala.twirl
sJavascriptReverseRouter.scala.twirl
sJavaWrappers.scala.twirl
sReverseRouter.scala.twirl
sRoutesPrefix.scala.twirl
and
sForwardsRouter.scala.twirl
jForwardsRouter.scala.twirl
But not sure, I think prefixing existing templates with s will break compatibility. So maybe just prefix the new templates just with j instead of writing out java. We do use this approach with some methods and variables in the code, maybe also here? What do you think?
There was a problem hiding this comment.
I think the names of templates are internal part and we are free to choice any pattern for them.
I like an idea with one letter s/j prefix. 👍
Adding another 24 hours ;) |
1cd6053 to
998e365
Compare
| "\n )," | ||
| ) | ||
| } | ||
| .filterNot(_ == ",") |
There was a problem hiding this comment.
Without this filternot the simple example
GET / controllers.HomeController.index(r: Request)
public Result index(Http.Request r) { ... }fails. Because ps.mkString("", ",\n ", ",") above will add the trailing , even if the list is empty, so we end up with call(, ...) which of course does not compile (This is more or less the equivalent of the .filterNot(_ == "()") in the scala method).
There was a problem hiding this comment.
OMG, it's so stupid checked a lot of cases with any type of params and forgot about case with only one Request param 🤦♂️
There was a problem hiding this comment.
Test for this case was added in this commit 99aad10
| public static final @{packageName.map(_ + ".").getOrElse("")}javascript.JavaScriptReverseRoutes.Reverse@controller @controller = new @{packageName.map(_ + ".").getOrElse("")}javascript.JavaScriptReverseRoutes.Reverse@(controller)(RoutesPrefix.byNamePrefix);} | ||
| @cb | ||
| } | ||
| @cb |
There was a problem hiding this comment.
I think we can unify [java]JavaWrapper.scala.twirl (specially because even in play scala it will be generated as java file).
I think we can just stupidly wrap the scala reverse routes in a ReverseRoutes class/object and also pass the prefix lamba as lambda and not lazy string.
With that two changes we should be able to just use one file.
Feels a bit cleaner IMHO as the differences between [java]JavaWrapper.scala.twirl is not really big.
There was a problem hiding this comment.
I'm not sure that I understood your idea for 100%, but I unified routes.java for both compilers. Can you check that (separated commit)?
I think we can just stupidly wrap the scala reverse routes in a
ReverseRoutesclass/object
Looks like If someone uses a reverse routes classes directly then these changes will break a source compatibility in new version. Is it ok to you?
998e365 to
36ecdb6
Compare
c68248b to
1e8e232
Compare

As main part of #13212