elm / url

Build and parse URLs. Useful for HTTP and "routing" in single-page apps (SPAs)
https://package.elm-lang.org/packages/elm/url/latest/
BSD 3-Clause "New" or "Revised" License
75 stars 43 forks source link

Url.Builder.absolute doesn't use Url.percentEncode but the Url.percentEncode docs claim that it does. #21

Closed jessta closed 5 years ago

jessta commented 5 years ago

The documentation for Url.percentEncode claims: "Use Url.Builder instead! Functions like absolute, relative, and crossOrigin already do this automatically! percentEncode is only available so that extremely custom cases are possible, if needed." https://package.elm-lang.org/packages/elm/url/latest/Url#percentEncode

But as you can see at https://github.com/elm/url/blob/master/src/Url/Builder.elm#L52-L54 Url.absolute doesn't use Url.percentEncode at all.

absolute : List String -> List QueryParameter -> String
absolute pathSegments parameters =
   "/" ++ String.join "/" pathSegments ++ toQuery parameters
evancz commented 5 years ago

If you follow the code around a bit more, you can see the following:

https://github.com/elm/url/blob/master/src/Url/Builder.elm#L179-L181

https://github.com/elm/url/blob/master/src/Url/Builder.elm#L192-L194

The implementation ensures that anything of the QueryParameter type is appropriately escaped, without escaping things where there is no point, like integers. So the toQuery function is guaranteed to be properly escaped without doing more computation than necessary.

From my reading of the spec, it is reasonable to escape query parameters but not to escape the path segments. Please open a new issue if you have (1) a specific SSCCE of something you think is not working and (2) looked through the spec (or implementations in a couple other languages) to confirm that the behavior you expect is widely expected.