Fixes #3313 Allow passing a ClassLoader to ResourceService and Webjar…

[ashwinbhaskar]

…Service

[rossabaker]

Thanks. I like the unit tests. Not very functional, but neither is the underlying Java API, and they prove the functionality nicely.

Some compat and API concerns below.

[rossabaker]

This is not binary compatible, so we'll have to relegate this to master. (And we need to figure out why our MiMa is not running. This burned us on 0.20.22.)

[ashwinbhaskar]

@rossabaker oh okay, do you want me to raise this PR against master?

[rossabaker]

Yes, unless we can figure out a way to make it binary compatible. We could if we left the original signature, and called the new one something other than apply. Like withClassLoader. But we'd probably want to deprecate that in 1.0 in favor of the builder.

[rossabaker]

Is there any reason to introduce another parameter rather than put this in Config?

I have a more radical idea: move this to the same kind of builder pattern we use for server builders, and unlike the case class config or default optional parameters, we'll be able to introduce more parameters without breaking compatibility in the future.

[rossabaker]

If this doesn't make any sense, I'd be happy to sketch one, and could follow the pattern on the other.

[ashwinbhaskar]

@rossabaker I did think about moving it to Config. But then I felt that config is meant to tune something. Config didn't feel like the right place to put classloader in. For example something like shouldUseDefaultClassLoader = true would make sense as part of Config. But having the classloader itself as part of config kind of felt out of place.
Wdyt?

[ashwinbhaskar]

@rossabaker A builder like this??

  class WebjarServiceBuilder[F[_]] {
    private var webjarAssetFilter: WebjarAssetFilter = _ => true
    private var cacheStrategy: CacheStrategy[F] = NoopCacheStrategy[F]
    private var classLoader: Option[ClassLoader] = None

    def wid(webjarAssetFilter: WebjarAssetFilter): WebjarServiceBuilder[F] = {
      this.webjarAssetFilter = webjarAssetFilter
      this
    }

    def wid(cacheStrategy: CacheStrategy[F]): WebjarServiceBuilder[F] = {
      this.cacheStrategy = cacheStrategy
      this
    }

    def wid(classLoader: ClassLoader): WebjarServiceBuilder[F] = {
      this.classLoader = Some(classLoader)
      this
    }

    def build(implicit F: Sync[F], cs: ContextShift[F]): HttpRoutes[F] = ???
  }

[rossabaker]

That's pretty close. This is pretty similar to a builder in an object-oriented language, but our backend "builders" are immutable. Something like:

// private constructor: we want people to call it like `WebjarServiceBuilder[F].blah.build`
class WebjarServiceBuilder[F[_]] private (
  webjarAssetFilter: WebjarAssetFilter,
  cacheStrategy: CacheStrategy[F],
  classLoader: Option[ClassLoader]) {

  // You don't strictly need this, but it means you don't need to repeat arguments in all your `with*` methods below. 
  private def copy(
    webjarAssetFilter: WebjarAssetFilter = webjarAssetFilter,
    cacheStrategy: CacheStrategy[F] = cacheStrategy,
    classLoader: Option[ClassLoader] = classLoader) =
    new WebjarServiceBuilder(webjarAssetFilter, cacheStrategy, classLoader)

  // Each field gets its own "with" setter.
  def withWebjarAssetFilter(webjarAssetFilter: WebjarAssetFilter) = 
    copy(webjarAssetFilter = webjarAssetFilter)

  // and so on for cacheStrategy and classLoader

  // Or maybe call this `.toRoutes`?
  def build = ???
}

object WebjarServiceBuilder {
  // If you have arguments with no sensible default, pass them to apply.
  // This is also a good spot for constraints, if you have any.
  def apply[F[_]] = new WebjarServiceBuilder(
     // Supply your default arguments here
     webjarAssetFilter = _ => true,
     cacheStrategy = NoopCacheStrategy[F],
     classLoader = None)
}

I haven't tried to compile any of that, but look at the other files named *Builder for working examples.

[ashwinbhaskar]

@rossabaker cool, got it

[ashwinbhaskar]

@rossabaker do you want to get rid of the class WebjarService.Config[F[_}] all together? That will impact the way it is interfaced with the outside world. Here is the line. The users of the library will now have to change their way of calling wejarService[F](..) method.

[ashwinbhaskar]

closing this pull request in favor of creating this pull request against master - #3373