Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify WebJars support #33495

Closed
wants to merge 1 commit into from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Sep 6, 2024

@bclozel
Copy link
Member

bclozel commented Sep 6, 2024

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.

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Sep 6, 2024
@vpavic
Copy link
Contributor Author

vpavic commented Sep 6, 2024

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 6, 2024
@bclozel
Copy link
Member

bclozel commented Sep 6, 2024

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?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 6, 2024
@vpavic
Copy link
Contributor Author

vpavic commented Sep 6, 2024

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 6, 2024
@vpavic
Copy link
Contributor Author

vpavic commented Sep 6, 2024

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
Copy link
Member

bclozel commented Sep 6, 2024

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

@bclozel bclozel closed this Sep 6, 2024
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Sep 6, 2024
@vpavic vpavic deleted the simplify-webjars-support branch September 6, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants