Simplify WebJars support

[vpavic]

#27619 (comment)

[bclozel]

It looks like this proposal is making WebJarsResourceResolver depend on two different types: WebJarVersionLocator (from org.webjars:webjars-locator-lite) and WebJarAssetLocator (from org.webjars:webjars-locator-core). Because those two types are used by public constructors, I think this class now needs both dependencies to be on the classpath to be loaded. Doesn't this defeats the purpose? Isn't it going to make things harder for maintenance and deprecation in the future?

Please advise.

[vpavic]

I think this class now needs both dependencies to be on the classpath to be loaded.

Can you clarify why do you believe this is the case?

Before submitting this PR, I built Framework snapshot with these changes locally and used it in a sample app to test both scenarios - with only org.webjars:webjars-locator-lite available on classpath, and with only org.webjars:webjars-locator-core available.

[bclozel]

I have just tried to create a sample application with the following dependencies:

dependencies {
	implementation 'org.springframework.boot:spring-boot-starter-web'
	implementation 'org.webjars:webjars-locator-lite'
	testImplementation 'org.springframework.boot:spring-boot-starter-test'
	testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
}

Trying to instantiate manually the resolver like so:

@SpringBootApplication
public class WebjarsApplication {

	public static void main(String[] args) {
		WebJarsResourceResolver webJarsResourceResolver = new WebJarsResourceResolver(new WebJarVersionLocator());
		SpringApplication.run(WebjarsApplication.class, args);
	}

}

Yields the following:

$ ./gradlew assemble

> Task :compileJava FAILED
/Users/bclozel/workspace/tmp/webjars/src/main/java/com/example/webjars/WebjarsApplication.java:13: error: cannot access WebJarAssetLocator
                WebJarsResourceResolver webJarsResourceResolver = new WebJarsResourceResolver(new WebJarVersionLocator());
                                                                  ^
  class file for org.webjars.WebJarAssetLocator not found
1 error

FAILURE: Build failed with an exception.

Am I missing something?

[vpavic]

Ah, I see - the problem is when instantiating resolver in the application code, which doesn't compile if both implementations are not present on the classpath at that time. Admittedly in my tests I was focused on scenarios where it's the Framework instantiating resolver based on what's available on the classpath.

But shouldn't it be possible to overcome this issue if constructor accepted simply Object for the midterm while both options are supported? Such constructor could be immediately deprecated, in favor of a factory method - potentially even adding one that allows WebJarsResourceResolver to be created using BiFunction<String, String, String> which would increase its ability to be reused with different locator implementations.

[vpavic]

This works for me:

public WebJarsResourceResolver(Object webJarLocator) {
	this.webJarLocator = switch (webJarLocator.getClass().getName()) {
		case "org.webjars.WebJarVersionLocator" -> ((org.webjars.WebJarVersionLocator) webJarLocator)::fullPath;
		case "org.webjars.WebJarAssetLocator" -> ((org.webjars.WebJarAssetLocator) webJarLocator)::getFullPathExact;
		default -> throw new IllegalArgumentException("Unsupported WebJar locator implementation");
	};
}

While such constructor isn't pretty, it's only for the midterm and doesn't break anyone while providing flexibility for the future while avoiding the following (IMO) downsides of the current solution:

• keeping the same inflexibility that was present until now (what happens if there's another WebJar locator implementation few years down the line?)
• code duplication
• you end up with awkwardly named LiteWebJarVersionLocator as your only implementation
• breaking the continuity (version control history wise) of WebJarsResourceResolver

[bclozel]

Thanks for the proposal, but I think we're fine with the current tradeoffs.