Ensure consistent behaviour for path and fullPath #19
Conversation
|
Thanks for tackling this. I think it makes sense to have the only difference between |
|
Thanks for the ping @jamesward. I am all for consistency, but I am not sure to be in favor of merging this PR as it is. The version check and removal in Maybe I miss some context, but I would recommend double checking why this support was introduced initially in If you are sure you want to do it, I think it should be applied on both cc @bclozel for advice. |
|
Hey @dodgex @jamesward I had a look at this PR and here's what I think. This change should not break Spring as we're first checking if the resource exists before delegating to webjars-locator. So we shouldn't be calling the library if the version is already present in the path. Now I think the main problem here is the lack of contract. the In summary, I would opt for the opposite: align path with the fullPath behavior and don't try and generate a webjar URL that will obviously fail at runtime. |
|
Thank you guys for your feedback. I agree with @bclozel that there is a lack of a contract. Here @jamesward should decide on a contract. Unfortunately the commit messages related to the lines with the version check did not seem to be very helpful to determine the need for the check.
The/My intent was to intentionally fail early when adding a WebJar with a hardcoded path to avoid the same problem later, when the WebJar dependency is upgraded. In my opinion, the chance to catch a 404 is higher when adding a new WebJar to the HTML as when later upgrading e.g. Bootstrap 5.3.2 to 5.3.3. but having the 5.3.2 in the HTML. But as the So unless the Spring Team decides to remove that check, this PR has no impact on Spring projects at all. For now I agree, with changing the PR to keep the check but still have Examples
CasesIn the following cases, the examples above are tested with and without the locator dependencies. ✅ = works (CSS loaded successfully)
No locator dependency
webjars-locator-core 0.59
webjars-locator-lite 1.0.1
webjars-locator-lite 1.0.2-SNAPSHOT (local build of this PR)
|
|
I updated the PR to re-add the version check to Just to be sure, I also tested the six test cases with a new build of this PR. As expected, there are no changes at all. webjars-locator-lite 1.0.2-SNAPSHOT (local build of updated PR)
|
|
And btw, in |
|
Thank you all for the thoughtful and thorough responses! I like where this is at. I think it is important we guide users to a place where upgrading WebJar deps won't break things at runtime. I do like @bclozel's suggestion to add better JavaDocs stating the "contract" - @dodgex can you take a stab at that? |
At this point I wish I had just answered I probably overshoot the desired goal by a mile or two, but I hope this is ok. Beside trying to add more details and some kind of "contract" to @jamesward please double and tripple proofread the updated JavaDoc to ensure no mistakes or bad formulations or whaterver. I am not a native english speaker and did a lot of iterations to the comments. I tried to proofread it as good as I can, but at some point you become blind to your own mistakes. |
| * | ||
| * <p>Custom WebJars can be registered by using a {@code webjars-locator.properties} file. See {@link WebJarVersionLocator} for details. | ||
| * | ||
| * @param webJarName The name of the WebJar (the Maven artifact ID) |
There was a problem hiding this comment.
@jamesward for official WebJars the webJarName (as in: the part in the path) is also the Maven artifact ID, but for custom webjars this is not necessarily true. In custom WebJars those names can differ. Our in-house WebJars have e.g. camera-module-webjar as Maven artifact but just camera-module in the resource path. There it would be the name give in the webjars-locator.properties
Not sure if this extra case should be mentioned/taken care of in the javadoc, or not mention it as it was before.
Also you added the (the Maven artifact ID) only to the version method, but not to the path and fullPath. I guess, that was not intentional?
| * | ||
| * <p>Custom WebJars can be registered by using a {@code webjars-locator.properties} file. See {@link WebJarVersionLocator} for details. | ||
| * | ||
| * @param webJarName The name of the WebJar (the Maven artifact ID) |
There was a problem hiding this comment.
Thanks for pointing that out. How about:
| * @param webJarName The name of the WebJar (the Maven artifact ID) | |
| * @param webJarName The directory name in the WebJar following `META-INF/resources/webjars` - Usually the same as the Maven artifact ID |
There was a problem hiding this comment.
Inspried by this suggestion I reworded the description a bit and added it to the other two methods.
Hope it is ok the way it is now. :)
| * locator.path("unknown", "some/file.css"); | ||
| * }</pre> | ||
| * | ||
| * <p><b>Note:</b> When the {@code filePath} starts with a version, that is not the correct version of the WebJar, this method returns a path with both versions. |
There was a problem hiding this comment.
I'm not sure we need to point out this implementation detail in the Javadoc.
There was a problem hiding this comment.
You are right. I was unsure on how much details we need, so I might have added a bit too much. Its gone now.
| * locator.path("bootstrap", "css/bootstrap.css"); | ||
| * | ||
| * // returns "bootstrap/3.1.1/js/bootstrap.js" as well | ||
| * locator.path("bootstrap", "3.1.1/css/bootstrap.css"); |
There was a problem hiding this comment.
Yep typos. Will fix that
|
Thanks all! I think this is looking good and I'm ready to merge and release |
This PR makes the behaviour of
pathandfullPathconsistent. This is done by removing the version check, infullPath. Also to not have the sameString.formatduplicated, i use thepathmethod insidefullPath.As this broke the related test, I updated the test and renamed the
full_path_exists_version_suppliedtofull_path_double_version_with_version_suppliedand addedpath_double_version_with_version_suppliedto have a similar test for path only.One thing to note here, is that this can and probably should be considered a minor breaking change. As this breaks cases where a user accidentally added a version to the referenced webjar resouce in thier html. Becaus of this I think maybe this should rather become a
1.1.0instead of a1.0.xrelease. Not sure if this kind of breakage deserverse a major bump.This also ensures that if Spring Boot upgrades to the version containing this does not silently land in
3.4.xbut only in3.5.0and thus can be mentioned in thier release notes.Fixes #18