magnars
Access assets using 'clojure.java.io/resource'
Instead of listing all the files in all the JARs on the classpath (with exception of ~/.m2/... which, btw, makes testing assets in such JARs on the REPL impossible), this PR finds the desired directory (either on the filesystem or within a JAR) and creates the file list in a more targeted manner.
I hope you can use this but I might be overlooking some use case where this approach breaks things.
Tests are passing.
This looks really great. I wonder, tho: is it a breaking change?
What happens if I ask for all assets in "public/scripts" - and this folder is present in both the file system and a jar? In the old version, all files would be included.
What is the URL protocol to a virtual folder on the class path?
What happens if I ask for all assets in "public/scripts" - and this folder is present in both the file system and a jar? In the old version, all files would be included.
This isn't the case here but can be added easily. There might be a problem, though, if the same path is available in multiple JARs - in which case, I assume, clojure.java.io/resource only returns one of those. I'll investigate and report back to you.
What is the URL protocol to a virtual folder on the class path?
The URL looks like this: jar:file:<path to JAR>!/<resource path>
Update: It looks like this is perfectly feasible using the current classloader to retrieve an enumeration of all resources of a given name. (And I have to update create-binary-asset and co. to work with URLs instead of strings.) I'll add the respective commits to this PR once I have them. :)
This should take care of #21, too.
I haven't had time to add tests targeted at the "same directory name at different classpath locations" use case yet but as far as I can tell the changes are non-breaking. Let me know what you think!
Sorry about the lack of feedback here. It's quite a big change, so I'll have to go through it thoroughly. I'm glad the tests are passing, tho.
There is breakage tho, since the publicly available load-asset multimethod has changed signatures, which is used by other plugins to add asset loaders.
It seems like this is the way to go either way, I just need to be diligent about such a big change. :)
No problem. You can always just cherry-pick whatever is useful to you since I'm sure there are different (maybe superior) ways of implementing this, and if you come up with one that is truly backwards-compatible I'm all in favor. :)
On another note, now that I thought about it a bit: does it really make sense to collect all those resources with the same path? If I ask the optimus middleware to retrieve /static/file.js (i.e. the raw version of the file) and there are two, which one do I get?
It should probably be the responsibility of the developer to choose unique names for files (or different path prefixes) and adjust the patterns accordingly. If I want to use an asset from a JAR I depend on I might want to know its name anyways.
A specific case where the current behaviour might cause headaches:
- I've deployed a library to Clojars that contains some JS assets, i.e.
/static/file.js. - I now want to simultaneously work on said library and one that has it as its dependency.
- I create a
checkoutsdirectory which means that there are two versions offile.json the classpath:checkouts/<lib>/resources/static/file.jsand<lib>.jar!/static/file.js.
If optimus uses both for creating a bundle there will be duplicate code. If optimus uses only one (either for the bundle or directly) it has to decide which one. It doesn't get easier if I introduce another file that resides at static/file.js in the project directory.
Sorry for rambling, I just think the current behaviour might cause some surprises after all.
It should probably be the responsibility of the developer to choose unique names for files (or different path prefixes) and adjust the patterns accordingly. If I want to use an asset from a JAR I depend on I might want to know its name anyways.
It should, and certainly this seems reasonable to expect for first-party artifacts (I find this works well in my projects). However, it might be worth seeing how Webjars, lein-npm or lein-bower resources appear in the classpath. (If nothing else, documenting some guidance for dealing with them would be the least Optimus could do.)
I just discovered that the way the solution is implemented now might pose problems with Uberjars. (Resources could not be found in some cases.) It's probably best to close this PR for now and maybe use it as a starting point when revisiting this issue.
I think the problem with Uberjars came from some other part of my codebase. Anyhow, I put directory-based resource lookup into cpath-clj since I needed the functionality in a different context, too. (Also, there are tests in that repository.)
I'll revisit this PR but I don't think it will be possible to maintain backwards-compatibility when introducing URLs as pointers to assets...
We're still on 0.x release, and this sounds like the right thing to do, so we can live with some breaking changes. Let me know how it goes.
Is there any chance of reviving this PR? Fortunately, we're still at 0.x; but the basic idea looks pretty sound.
Yeah, I would like this revived as well. @xsc, any chance you would be able to reproduce the breaking change in tests so we could get to work on fixing it?