Add support for webjars-locator.properties #15

dodgex · 2024-10-11T12:35:12Z

With this PR I want to bring up a possible implementation for supporting legacy and custom webjars with webjars-locator-lite. I started the related discussion in #13.

Unfortunately there was no decision on how to implement this feature in the related issue, therefore I tried to come up with something based on the suggestions.

The Implementation provided by this PR looks for META-INF/webjars/locator.properties files on the classpath and then reads them into a Properties instance. I decided to use a pattern of WEBJAR_NAME.version as key in the properties file to have the option to add other properties in the future (and be more specific about the purpose of the property -> providing a version). The value for each key has to be the version of the webjar as used in the resulting path.

When reading the locator.properties the .version suffix is removed and the given version is put into the cache. Before adding a given webjar version to the cache, it checks if the resulting path exists.

The files are loaded in whatever order LOADER.getResources() returns them.

With this change, it is possible to support any legacy (e.g. bower) or custom webjar, as long as the basic path structure is valid (META-INF/resources/webjars/WEBJAR/VERSION/any/folder/or.file)

dodgex · 2024-10-11T12:35:46Z

src/main/java/org/webjars/WebJarVersionLocator.java

@@ -25,17 +28,22 @@ public class WebJarVersionLocator {
    private static final String NPM = "org.webjars.npm/";
    private static final String PLAIN = "org.webjars/";
    private static final String POM_PROPERTIES = "/pom.properties";
+    private static final String LOCATOR_PROPERTIES = "/locator.properties";
+
+    private static final String CACHE_KEY_PREFIX = "version-";


I added this to avoid duplication of the string

dodgex · 2024-10-11T12:36:54Z

src/main/java/org/webjars/WebJarVersionLocator.java

+                }
+            }
+        } catch (IOException e) {
+            throw new RuntimeException("unable to load locator properties", e);


Not sure if this exception should be rethrown, logged or just ignored (similar to version()). For now I opted for throwing.

dodgex · 2024-10-11T12:39:03Z

src/test/java/org/webjars/WebJarVersionLocatorTest.java

                    return value;
                });
                return cache.computeIfAbsent(key, inspectableFunction);
            }
        }

        final WebJarVersionLocator webJarVersionLocator = new WebJarVersionLocator(new InspectableCache());
+        // enable inspection after webJarVersionLocator has been constructed, to ignore lookups caused by loading locator.properties
+        shouldInspect.set(true);


My first Idea was to modifiy the assertEquals calls for numLookups but I think that this is a better solution.

jamesward · 2024-10-12T21:28:40Z

src/main/java/org/webjars/WebJarVersionLocator.java

+                }
+            }
+        } catch (IOException e) {
+            throw new RuntimeException("unable to load locator properties", e);


I think that this Exception should only happen if the the locator.properties was found but could not be read. So we should be good if there is no locator.properties - is that right?

Yeah, LOADER.getResources returns an empty Enumeration when there are no files found. I verified that by removing the locator.properties from test resources.

jamesward · 2024-10-12T21:29:56Z

Thanks for tackling this! Overall I think this looks good. Does this address everything you need for #13?

dodgex · 2024-10-13T08:55:48Z

Yeah, I am pretty sure, that this should be a proper solution for #13.

Due to the fact, that all webjars should have thier content in META-INF/resources/webjars/{webjar}/{version}/* providing pairs of webjar and version to the locator and its cache is enough to find the resources. This way we can bypass the need to scan in META-INF/maven/ for the pom.properties of non standard group ids. And at the same it also fixes the fact that our custom webjars have a diffrent artifact id (camera-module-webjar) than the webjar name in the resources folder (camera-module).

I'm not sure about the performance impacts of the two variants, but maybe the locator.properties could even be something to add to new webjars for the future by default and have the pom.properties as fallback for older webjars? But that is another topic. :)

jamesward · 2024-10-13T20:52:22Z

Thanks! Since this is a change we'd like to have integrated into Spring, @dsyer want to review?

dsyer · 2024-10-14T06:46:08Z

If I’m reading it correctly this locator properties file kicks in whenever it is present (and mostly it would not be). So users don’t have to do anything unless they want to provide locator properties themselves, but libraries might do that for them most commonly. If all that is correct there’s nothing to be done to integrate with existing usage.

bclozel · 2024-10-14T07:24:43Z

I don't know if supporting GraalVM native images is in the scope of this library; if that's the case, I would suggest adding the following file src/main/resources/META-INF/native-image/org.webjars/webjars-locator-lite/resource-config.json with content:

{
    "resources": {
        "includes": [
            {
                "pattern": "\\QMETA-INF/webjars/locator.properties\\E",
                "condition": {
                    "typeReachable": "org.webjars.WebJarVersionLocator"
                }
            }
        ]
    }
}

sdeleuze · 2024-10-14T07:29:56Z

+1 for @bclozel proposal as well as doing a real test of a Spring application compiled to native to check this PR does not break native support (which was one of the goal of webjars-locator-lite).

dodgex · 2024-10-14T16:34:44Z

If I’m reading it correctly this locator properties file kicks in whenever it is present (and mostly it would not be). So users don’t have to do anything unless they want to provide locator properties themselves, but libraries might do that for them most commonly. If all that is correct there’s nothing to be done to integrate with existing usage.

@dsyer Correct. As of now, the file would only be present if a user is adding it to support legacy webjars that would otherwise not be found, or by non-standard 3rd-party webjars. In theory, official webjars COULD provide the file going forward, but that is up to @jamesward or the webjars team to decide.

Regarding native-image, I think if it is just about adding that json file, I would say we should go for it. but that again is up to james to decide.

@bclozel @sdeleuze as I have no experience with graalvm/native-image at all, it would be awesome if one of you guys could help out with doing a native test for this.

jamesward · 2024-10-15T04:38:44Z

Thanks all! Yes, we definitely want to make sure this is all compatible with native image. So a test would be great. It's a bit of a lift and we should do it. In the meantime, question for @bclozel and @sdeleuze - if we include the resource-config, it is ok if the referenced file (locator.properties) comes from outside this library, right?

jamesward · 2024-10-15T04:39:44Z

btw, thanks all! you're amazing and I really appreciate your support!

bclozel · 2024-10-18T15:25:02Z

Given the extent of the native image metadata, I'm not sure it's worth building a native image test in the CI. With upcoming GraalVM versions, I think you should be able to test things with a non-native binary and still check that your metadata is applied as expected.

As for the locator.properties configuration, yes, this implies that the file can be anywhere in the classpath when the native application is built.

dodgex · 2024-10-19T08:23:34Z

I'm not sure it's worth building a native image test in the CI.

My interpretation of the "doing a real test", was more like build a small app (or use an existing) and test if this works there. not a full CI test that runs always. although this could make sense to have this in long term to ensure compat/not breaking stuff.

dodgex · 2024-10-19T11:08:32Z

I gave testing this in a project a try and started with testing a project using webjars-locator-core and first of all, I found that it does not like the locator.properties file inside META-INF/resources/webjars. It crashes in findWebJars while trying to extract the webjar name from the found resource path due to a missing /. Either the core locator needs an update to ignore the locator.properties file, or we should change the location of the file. e.g META-INF/resources/webjars.locator.properties or META-INF/resources/webjars-locator.properties.

After that I moved on and tried to get this working in GraalVM, or better said, get this to FAIL.

First I had the custom webjars resources + locator.properties in the project module itself. It found the properties file and thus the version. -> working
Then I moved the resources into a separate maven module in the same. Still working.
Finally I moved the locator.properties into the webjars-loctor-lite project/jar and used a bower based webjar. Again, working.

All this, without having the resource-config.json added.

Again, I am not familiar with GraalVM so I might have done something "wrong" for it to work or fail in this case. I tested using Spring Boot 3.4.0-M3, with the native-maven-plugin and building with mvn clean package -Pnative and ran the generated binary from target.

My assumption is, that this might be related to the spring-boot:3.4.0-M3:process-aot build step seemingly to start the app. And as the locator is registered at some point during startup/init and it loads the properties files in the constructor it maybe "registers" the files as loaded and required to be packaged in the native image?

jamesward · 2024-10-19T13:24:28Z

Thanks @dodgex for doing all this testing! Really good find with webjars-locator-core as that would have been a big problem. Can you move the location to META-INF/resources/webjars-locator.properties ?

As for not needing the resource-config.json I think you are right that it has to do with how Spring automates some of the GraalVM config. But I'll let the Spring folks clarify.

…loading webjars-locator.properties

dodgex · 2024-10-19T13:47:28Z

Amended the code and commit messages to use META-INF/resources/webjars-locator.properties instead of META-INF/resources/webjars/locator.properties.

dodgex · 2024-10-29T13:38:26Z

@bclozel @sdeleuze can you guys clarify if my assumption in #15 (comment) regarding the properties file in native is correct? I mean we can still add the resource-config.json just to be sure, but understanding the behaviour would be helpful. :)

sdeleuze · 2024-10-29T14:31:50Z

Spring is not expected to add blindly resource files, so you should probably check the resource-config.json file that the Spring AOT support is generated:

Maven: target/spring-aot/main/resources/META-INF/native-image/.../resource-config.json
Gradle: META-INF/native-image/.../resource-config.json

Quickly checking on Petclinic, it looks like WebJars entries are indeed added:

        "pattern": "\\QMETA-INF\/resources\\E"
      },
      {
        "pattern": "\\QMETA-INF\/resources\/\\E.*"
      },
      {
        "pattern": "\\QMETA-INF\/resources\/webjars\\E"
      },
      {
        "pattern": "\\QMETA-INF\/resources\/webjars\/\\E.*"
      },

I guess that's added via the values we configure for the resource handlers in Spring Boot, so indeed it looks like for Spring you should be good. So up to you to decide if you want to provide native support for webjars-locator.properties and WebJars resources without a framework like Spring. Looks like a different concern with a wider scope, so I think you should be good without an additional resource-config.json.

dodgex · 2024-10-29T16:24:16Z

Thank you for your quick response! :)

@jamesward now it is up to you to decided, if the resource-config.json should be added in for native support in non spring environments or not. Actually, for me it looks like, that devs using webjars locator in native without spring have to provide the config anyway right now - even without the changes in this PR.

And I would say, as soon as you decide to add the file, it should include all entries to find webjars. not just the locator file. e.g. the stuff that is currently generated by the Spring AOT support as sdeleuze listed above in addition to the properties file.

jamesward · 2024-10-29T17:42:25Z

Thanks all! If there aren't any downsides to including the config, then let's do it. Currently I don't see any but let me know if I'm missing something.

dodgex · 2024-10-31T14:24:31Z

I added the JSON in 688013c, even though, this commit actually might be out of scope of the actual PR. But as we discussed it here i think it still fits and there is no need for another PR.

I hope this is whatever is needed. I added an entry for the webjars-locator.properties file, as well as the contents of the webjars/ resource path. and (assuming it might be necessary?) I also added the pom.properties files for all artifacts from the groupIds org.webjars and org.webjars.npm. For all cases, I included the typeReachable condition mentioned in #15 (comment) to ensure that the original webjars scanning still works:

jamesward · 2024-10-31T16:30:21Z

Thank you for all the work on this! I'm releasing 1.0.1 now.

dodgex · 2024-10-31T20:40:10Z

Awesome, I am glad we got this together! :)

jamesward · 2024-11-01T17:13:57Z

Thank you all!

Extract constant CACHE_KEY_PREFIX

4c44ceb

dodgex commented Oct 11, 2024

View reviewed changes

jamesward reviewed Oct 12, 2024

View reviewed changes

dodgex mentioned this pull request Oct 13, 2024

Upcoming release of webjars-locator-lite spring-projects/spring-framework#33693

Closed

dodgex added 3 commits October 19, 2024 15:42

Add support for webjars-locator.properties file

54556a3

Add tests for webjars configured by webjars-locator.properties

b59602d

Update cache_is_populated_on_lookup test to ignore webjars cached by …

5335fea

…loading webjars-locator.properties

dodgex force-pushed the 13-locator-properties branch from 0e2cb76 to 5335fea Compare October 19, 2024 13:44

dodgex changed the title ~~Add support for locator.properties~~ Add support for webjars-locator.properties Oct 20, 2024

dodgex force-pushed the 13-locator-properties branch from f9031b4 to e4a929d Compare October 31, 2024 14:18

add resource-config.json

688013c

dodgex force-pushed the 13-locator-properties branch from e4a929d to 688013c Compare October 31, 2024 14:19

jamesward merged commit 64cac7b into webjars:main Oct 31, 2024
1 check passed

dodgex deleted the 13-locator-properties branch October 31, 2024 20:39

This was referenced Nov 28, 2024

Support for additional npm locations considered? #13

Closed

Add info about webjars-locator.properties to README.md #17

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for webjars-locator.properties #15

Add support for webjars-locator.properties #15

dodgex commented Oct 11, 2024

dodgex Oct 11, 2024

dodgex Oct 11, 2024

dodgex Oct 11, 2024

jamesward Oct 12, 2024

dodgex Oct 13, 2024

jamesward commented Oct 12, 2024

dodgex commented Oct 13, 2024

jamesward commented Oct 13, 2024

dsyer commented Oct 14, 2024

bclozel commented Oct 14, 2024

sdeleuze commented Oct 14, 2024

dodgex commented Oct 14, 2024

jamesward commented Oct 15, 2024

jamesward commented Oct 15, 2024

bclozel commented Oct 18, 2024

dodgex commented Oct 19, 2024

dodgex commented Oct 19, 2024

jamesward commented Oct 19, 2024

dodgex commented Oct 19, 2024

dodgex commented Oct 29, 2024

sdeleuze commented Oct 29, 2024

dodgex commented Oct 29, 2024

jamesward commented Oct 29, 2024

dodgex commented Oct 31, 2024

jamesward commented Oct 31, 2024

dodgex commented Oct 31, 2024

jamesward commented Nov 1, 2024

Add support for webjars-locator.properties #15

Add support for webjars-locator.properties #15

Conversation

dodgex commented Oct 11, 2024

dodgex Oct 11, 2024

Choose a reason for hiding this comment

dodgex Oct 11, 2024

Choose a reason for hiding this comment

dodgex Oct 11, 2024

Choose a reason for hiding this comment

jamesward Oct 12, 2024

Choose a reason for hiding this comment

dodgex Oct 13, 2024

Choose a reason for hiding this comment

jamesward commented Oct 12, 2024

dodgex commented Oct 13, 2024

jamesward commented Oct 13, 2024

dsyer commented Oct 14, 2024

bclozel commented Oct 14, 2024

sdeleuze commented Oct 14, 2024

dodgex commented Oct 14, 2024

jamesward commented Oct 15, 2024

jamesward commented Oct 15, 2024

bclozel commented Oct 18, 2024

dodgex commented Oct 19, 2024

dodgex commented Oct 19, 2024

jamesward commented Oct 19, 2024

dodgex commented Oct 19, 2024

dodgex commented Oct 29, 2024

sdeleuze commented Oct 29, 2024

dodgex commented Oct 29, 2024

jamesward commented Oct 29, 2024

dodgex commented Oct 31, 2024

jamesward commented Oct 31, 2024

dodgex commented Oct 31, 2024

jamesward commented Nov 1, 2024