Java support for routes compiler by ihostage · Pull Request #13213 · playframework/playframework · GitHub
Skip to content

Java support for routes compiler#13213

Open
ihostage wants to merge 4 commits intoplayframework:mainfrom
ihostage:java-routes-compiler
Open

Java support for routes compiler#13213
ihostage wants to merge 4 commits intoplayframework:mainfrom
ihostage:java-routes-compiler

Conversation

@ihostage
Copy link
Copy Markdown
Member

@ihostage ihostage commented Mar 20, 2025

As main part of #13212

@ihostage ihostage requested a review from mkurz March 20, 2025 13:53
@ihostage ihostage force-pushed the java-routes-compiler branch from 6c60e32 to ef7f4d0 Compare March 20, 2025 13:59
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will implement adoption to this feature for Gradle Plugin in the separated PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mkurz mkurz moved this from 🆕 New to 🏗 In Progress in Play 4.0 (due 30st June 2025) Mar 20, 2025
@ihostage ihostage force-pushed the java-routes-compiler branch from ef7f4d0 to 1cd6053 Compare March 20, 2025 15:40
@ihostage ihostage marked this pull request as ready for review March 20, 2025 16:11
@ihostage ihostage moved this from 🏗 In Progress to 👀 In review in Play 4.0 (due 30st June 2025) Mar 20, 2025
@mkurz
Copy link
Copy Markdown
Member

mkurz commented Mar 20, 2025

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

javaJava?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It looks not good, but It's just a java prefix for all existed templates.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

@mkurz
Copy link
Copy Markdown
Member

mkurz commented Mar 21, 2025

Will take a look in the next 24 hours

Adding another 24 hours ;)

"\n ),"
)
}
.filterNot(_ == ",")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, it's so stupid checked a lot of cases with any type of params and forgot about case with only one Request param 🤦‍♂️

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ReverseRoutes class/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?

@ihostage ihostage force-pushed the java-routes-compiler branch from 998e365 to 36ecdb6 Compare March 26, 2025 12:55
@ihostage ihostage requested a review from mkurz March 28, 2025 15:31
@mkurz
Copy link
Copy Markdown
Member

mkurz commented Mar 31, 2025

@ihostage ihostage force-pushed the java-routes-compiler branch from c68248b to 1e8e232 Compare October 20, 2025 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

3 participants