add cache

[jamesward]

Note that I wanted to preserve a way to use the locator statically so there is now a WebJarVersionLocator.DEFAULT for that.

[dsyer]

Kind of OK by me. See comments.

[jamesward]

Thanks @dsyer for the review!

[sdeleuze]

I can live with it, but I am not a big fan of the DEFAULT static instance:

• I don't think WebJarAssetLocator from webjars-locator-core had one
• I expect a lot of users just missing it and using new WebJarVersionLocator() leading to 2 instances created with 1 not used
• If we are afraid users doing new WebJarVersionLocator() for each request, we could maybe just remove the static field and document this properly in the README (like webjars-locator-core does) and in the Javadoc.

Otherwise looks to to me.

[jamesward]

Thanks @sdeleuze. Yeah, my concerns were that'd it is too easy to create a new cache on each usage. I'm not sure if there is a better design which has a static default and a way to override it. Any typical patterns for this?

[sdeleuze]

I don't think there is a good "self-contained" way to do that, by design. But I am not sure the situation is different than WebJarAssetLocator which had heavy resource scanning at initialization. I would even say that the cost of a misuse is lower now.

My recommandation would be to only provide the constructor, without the static field, and document clearly on both Javadoc and README that:

• WebJarVersionLocator is thread-safe
• That a single instance (singleton) is expected to be created, either via a static field (a code sample showing a WebJarVersionLocator static field on user class can be provided) or via other mechanism like dependency injection.

This is also what is done for very popular dependency of the ecosystem like Jackson ObjectMapper.

[jamesward]

I guess in Java that is the best we can do. :) I'll get those changes done today.

[jamesward]

Changes are in. LMKWYT. Thanks!

[sdeleuze]

Awesome, looks good to me for the singleton removal.

The only remaining ask would be to make PROPERTIES_ROOT, NPM, PLAIN and POM_PROPERTIES static like what you did for WEBJARS_PATH_PREFIX (I think this was the spirit of @dsyer review as he mentioned "These could (should) still be static, right?" even if I was also confused by the fact the feedback in the code review was just on WEBJARS_PATH_PREFIX).

[jamesward]

Cool. I've changed that. And what about:

private final ClassLoader LOADER = WebJarVersionLocator.class.getClassLoader();

Should it be static as well?

[sdeleuze]

Indeed it can be static as well.

[vpavic]

@jamesward do you really anticipate that someone would want to use a custom cache implementation with WebJarVersionLocator?

It seems to me that caching can easily be an internal implementation detail, which then removes the need for WebJarCache abstraction and additionally allows your cache interactions to be atomic (by leveraging ConcurrentHashMap#computeIfAbsent).

[jamesward]

Good point. If the Spring's usage doesn't foresee plugging in a custom cache then we could make it internal. Question on ConcurrentHashMap#computeIfAbsent - I imagine that a common situation using the locator would be that multiple requests for the same WebJar will come in at the same time (due to parallel loading from the browser). Would computeIfAbsent essentially do a lock so that only 1 of the concurrent requests would do the actual lookup?
cc @sdeleuze

[sdeleuze]

There is indeed likely something to refine here.

Would computeIfAbsent essentially do a lock so that only 1 of the concurrent requests would do the actual lookup?
For the initial request yes, see https://stackoverflow.com/a/10589196/1092077 for more details.

I created a dedicated new issue, let's maybe continue the discussion there: #8