-
Notifications
You must be signed in to change notification settings - Fork 38.4k
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
Simplify WebJars support #33495
Conversation
It looks like this proposal is making Please advise. |
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 |
I have just tried to create a sample application with the following dependencies:
Trying to instantiate manually the resolver like so:
Yields the following:
Am I missing something? |
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 |
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:
|
Thanks for the proposal, but I think we're fine with the current tradeoffs. |
#27619 (comment)