Skip to content
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

Access assets using 'clojure.java.io/resource' #20

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xsc
Copy link
Contributor

@xsc xsc commented Apr 29, 2014

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.

@magnars
Copy link
Owner

magnars commented Apr 30, 2014

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?

@xsc
Copy link
Contributor Author

xsc commented Apr 30, 2014

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>

@magnars
Copy link
Owner

magnars commented Apr 30, 2014

Thanks for looking into this!

@xsc
Copy link
Contributor Author

xsc commented May 2, 2014

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.

@magnars
Copy link
Owner

magnars commented May 2, 2014

Excellent. :)

@xsc
Copy link
Contributor Author

xsc commented May 2, 2014

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!

@magnars
Copy link
Owner

magnars commented May 5, 2014

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. :)

@xsc
Copy link
Contributor Author

xsc commented May 5, 2014

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. :)

@xsc
Copy link
Contributor Author

xsc commented May 5, 2014

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 checkouts directory which means that there are two versions of file.js on the classpath: checkouts/<lib>/resources/static/file.js and <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.

@radhikalism
Copy link
Collaborator

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.)

@xsc
Copy link
Contributor Author

xsc commented May 16, 2014

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.

@magnars
Copy link
Owner

magnars commented May 16, 2014

Did you find out in what cases the uberjar made for problems?

@xsc
Copy link
Contributor Author

xsc commented Jun 7, 2014

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...

@magnars
Copy link
Owner

magnars commented Jun 7, 2014

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.

@lvh
Copy link

lvh commented Oct 6, 2015

Is there any chance of reviving this PR? Fortunately, we're still at 0.x; but the basic idea looks pretty sound.

@magnars
Copy link
Owner

magnars commented Oct 6, 2015

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants