WebJarsUtil needs a rewrite

[mberndt123]

Hi,

I'm sorry for the inflammatory title, but I think I have good reasons to hold this position.

• Say you've got a webjar which contains two files: foo/1.0.0/index.js and foo/1.0.0/bar/index.js. Creating a script tag for the latter is trivial, you just do @Html(webJarsUtil.script("foo", "index.js")). But for the former it's difficult, because "index.js" is can also be understood to be a partial path to the latter, hence you get a MultipleMatchesException. You can work around it by specifying the version number, but then things will break when you upgrade the library. The real problem here is that there's no convenient way to separate the two different things that webJarsUtil does, which are 1. locating an asset and 2. transforming it into something useful, like a script or css element. You have to do something like @Html(webJarUtils.script(webJarUtils.localOrCdnUrl(webJarUtils.fullPath("foo", "index.js")))) to address my use case (I did hit this case in practice a while ago).
• WebJarsUtil.locate(String, String) is also inefficient since it calls WebJarAssetsLocator.locate(String, String), which calls WebJarAssetsLocator.filterPathByIndex which traverses the complete list of all assets in all webjars, which can be slow.
• WebJarsUtil encourages poor coding practices by returning HTML tags as a String rather than Play's Html type. This forces users to write @Html(webJarsUtil.css(…)) rather than just @webJarsUtil.css(…), and using @Html is generally discouraged.
  • the *withParams methods (scriptWithParams etc.) don't escape these parameters properly, so they're not safe to use.
  • I'm not sure it's wise choice to generate a comment when a WebJar is not found. In dev and test mode, I think the library should just scream loudly at the user (i. e. just throw an exception) so they can fix it. A comment is too easy to miss and can lead to a very annoying debugging experience.
  • WebJarsUtil#fullPath is just a reimplementation of WebJarAssetLocator.getFullPathExact

As far as I can see, there's no way to fix this short of essentially rewriting WebJarsUtil.

• It should use Twirl templates for the HTML tags (which will automatically create elements of type Html rather than String and will also escape special characters correctly).
• There should be a new type (we could call it WebjarAsset) which is returned from the locate and fullPath methods. Methods like localOrCdnUrl, tag, script, path should be moved to this new type and operate on the previously located path. This separates more clearly the two separate steps of 1. locating an asset and 2. transforming it to useful Html.

I'd be willing to implement this (WebJarsUtil isn't that huge after all), but first I'd like to hear what you think about this proposal.

[jamesward]

Thanks for the feedback! Agree that there is definitely some sub-optimal stuff here. I'd love any PRs to address this. :)

[jamesward]

Thanks a ton! This has been released as 2.6.3 and will be available shortly. Bonus points if you update: https://github.com/webjars/webjars/blob/master/app/views/documentation.scala.html :)

[mberndt123]

Hey @jamesward, allright, I hope I'll get around to that tomorrow