Support for additional npm locations considered?

dodgex commented 3 months ago

Hello there,

with Spring Boot deprecating webjars-locator-core in 3.4 and considering to remove support in a later version I realized, that this new locator only considers org.webjars and org.webjars.npm within META-INF/maven/ as valid webjar locations.

Beside the fact, that we (afaik) still have some projects using org.webjars.bower that for sure should be replaced asap anyway, we also have some custom in-house webjars where the pom.properties is located in a path like META-INF/maven/our.domain/project/pom.properties

Now my question is: Is there any plan to add some kind of support of adding/configuring custom pathes for this lookup? Or do we have to migrate our webjars to use either org.webjars or org.webjars.npm?

jamesward commented 3 months ago

Thanks for bring this up. A few thoughts... The Bower WebJars are deprecated and general support will be impossible without classpath scanning because the path can have anything, like: https://www.webjars.org/listfiles/org.webjars.bowergithub.datatables/dist-datatables-jqueryui/1.11.3

The Bower Original WebJars could be supported because their paths are like the NPM WebJars: https://www.webjars.org/listfiles/org.webjars.bower/jquery/3.7.1

Whatever we come up with for path customization could be used to add support for the Bower Original WebJars.

So to support the additional locations, we could read a file from like: META-INF/webjars/locator.properties

This could provide configurable locations but we will need to design that. Some considerations:

How do we configure the "defaults" for Classic and NPM WebJars? If we find a META-INF/webjars/locator.properties should it add to the defaults, or should we require those mappings be configured manually?
We's have to support multiple mapping files but there is an unfortunate implicit precedence based on classpath ordering.
From what I remember, we currently first look for the file in the Classic location, then fallback to the NPM location. As more locations are added I think we want to avoid the fallback-to-find approach but I'm not sure how to do that as I don't think we currently have anything to key off to know which location to look in. Maybe the fallback approach is the only viable one and we need to rely on the cache to avoid overhead.

Any other ideas for how we can accomplish this?

dodgex commented 3 months ago

Sorry for the later response, I wanted to reply earlier, but last two days where... bad.

After digging a bit more in the code of this new Locator and the old (core) one, i realized that our internal jars have even more "problems". As for example one of our maven artifacts is called "camera-module-webjar" and thus has its pom.properties in META-INF/maven/our.domain/camera-module-webjar/ while the files are in META-INF\resources\webjars\camera-module\{version}. But i guess, that is our "fault" and should not necessarily be on the locator to handle.

My first thought was to be able configure the mappings manually. The locator provides an API to add additional search paths, and Spring Boot (in my case) could provide properties to configure those via application.yml or via a Bean or something.

Manually configuring the paths would bypass the potential precedence problem that could happen with detecting the META-INF/webjars/locator.properties file.

On the other hand, would that require that every project that depends on a "non-standard" webjar would have to configure the paths.

With a META-INF/webjars/locator.properties our custom webjars could keep thier artifactId while not having the -webjar in the resource path as mentioned above, by even providing full path to the files in any way?

And if the file supports "configuring" multiple webjars, also the bower webjars could be supported/configured by the users project?

The most basic file layout could be something like:

# e.g. our custom webjar.
camera-module=META-INF/resources/webjars/camera-module/1.2.3

# the bower examples from above
datatables-jgui=META-INF/resources/webjars/datatables.net-jqui/
jquery=META-INF/resources/webjars/jquery/3.7.1/

Depending on when the scan for the properties files happens, this still has some initial overhead, but the result could be cached and it is still faster than the old scannig in webjars-locator-core.

And with the currently implemented fallback based lookup, existing org.webjars and org.webjars.npm artifacts can still be supported.

And coming back to the precedence problem once again, isnt this a potential problem already? When I am not mistaken, the webjars-locator-core only considers the first found webJarName to be added to the "cache".

dodgex commented 1 month ago

Hello @jamesward, did you have any time to think about this issue and/or my last suggestion?

If we can find a concept that works for you, i would try to implement it via a PR. Maybe we can get this done before Spring Boot 3.4 releases in November. so that they potentially upgrade thier management and the old and/or custom webjars do not get broken (or at least "easily" fixable).

dodgex commented 3 days ago

Hey @jamesward,

I was about to update the readme to add some documentation for the webjars-locator.properties added in #15, but I found that a lot (if not all; e.g META-INF/resources/webjars/bootstrap/dist/css/bootstrap.min.css) of the "bowergithub" WebJars have no version in thier resource path. I did a short test and found that there are two isEmpty checks on the version, so that it just not "simply works" by having a blank version in the properties file.

The question is, do we/you care about support for legacy bowergithub jars? For me the main focus of this issue was to enable custom webjars, having the lucky side effect of supporting legacy "bower" jars in the same way. But if you say we should go the extra mile to also allow webjars that do not have a version part in the path, I am willing to give it a shot.

Also, kind of offtopic for this issue but maybe something to look at: fullPath and path have slightly inconsitent behaviour for cases where exactPath contains the version. I found this as I was about to get the "bowergithub" case working.

Due to the amount of change needed I wanted to assure that you want changes to support webjars without version part, before continuing.

My opinion: While in theory it would be good to support the old stuff if not too much effort. I think it might not be as worthy as they are going to be unlisted from webjars.org in a few months and webjars-locator-core will still be available until one can upgrade the webjar usage.

For now I will stick to updating the readme, but please let me hear your thoughts on this. :)

jamesward commented 6 hours ago

Thanks for finding this and digging into it! I'm with you that if the amount of work were trivial, I'd say we add support for the bowergithub WebJars. But it sounds like that isn't the case. And the fallback to webjars-locator-core is fine. So we should probably just document the incompatibility and see if anyone else is disturbed enough by this, that they send a PR.

dodgex commented 2 hours ago

Okay. Let's wait and see. :) I think, this issue can then be considered as "fixed".

Regarding the path and fullPath methods, I have opend another issue https://github.com/webjars/webjars-locator-lite/issues/18

webjars / webjars-locator-lite

Support for additional npm locations considered? #13