Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix the Javascript Reverse router. Problem with default values. #888

Merged
merged 1 commit into from

3 participants

@guillaumebort

The code generated in the Javascript reverse router in wrong for arguments with a default value.

Try:

GET  /       controllers.Application.index(limit: Int =? 10)

And try Js reverse router for this call:

Router.controllers.Application.index()

It gives

/?10

Because anyway the default values are fixed server side, the client doesn't need to repeat the value. The simplest fix is to omit completely non-specified arguments that have a default value server side.

@jroper
Owner

Looks good to me.

@jroper jroper referenced this pull request from a commit in typesafehub/playframework
@jroper jroper [#888] Implemented new HTML escaping 78ba56e
@jroper jroper merged commit 887fc04 into from
@julienrf
Collaborator

Why is it different for parameters with fixed value? Their value is fixed by the routing process on server side, right?
So maybe we should just get rid of all the JavaScriptLitteral stuff?

@julienrf
Collaborator

Nothing about my remark?

@guillaumebort

Yes it should be the same.

@julienrf
Collaborator

I just took a look and I think JavascriptLitteral is needed to make unambiguous the reverse routing of an action that is associated with several URLs:

GET    /foo  p.c.a(foo = "foo")
GET    /bar  p.c.a(foo)

When I reverse route p.c.a("bar") in JavaScript we must to check, from the client-side, the value of the parameter in order to generate the right URL (here, /bar).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  framework/src/routes-compiler/src/main/scala/play/router/RoutesCompiler.scala
@@ -596,7 +596,7 @@ object RoutesCompiler {
queryParams.map { p =>
("(\"\"\" + implicitly[QueryStringBindable[" + p.typeName + "]].javascriptUnbind + \"\"\")" + """("""" + p.name + """", """ + localNames.get(p.name).getOrElse(p.name) + """)""") -> p
}.map {
- case (u, Parameter(name, typeName, None, Some(default))) => """(""" + localNames.get(name).getOrElse(name) + " == null ? \"\"\" + implicitly[JavascriptLitteral[" + typeName + "]].to(" + default + ") + \"\"\" : " + u + ")"
+ case (u, Parameter(name, typeName, None, Some(default))) => """(""" + localNames.get(name).getOrElse(name) + " == null ? null : " + u + ")"
case (u, Parameter(name, typeName, None, None)) => u
}.mkString(", "))
Something went wrong with that request. Please try again.