search

documentcloud / jammit

Industrial Strength Asset Packaging for Rails
http://documentcloud.github.com/jammit/
MIT License
1.16k stars 197 forks source link

Wrong place to be defining ASSET_ROOT and PUBLIC_ROOT #38

Closed jacquescrocker closed 14 years ago

jacquescrocker commented 14 years ago

Seems like a terrible idea to be using Rails.root and Rails.public path directly in your jammit.rb. Assigning to a constant no less so ruby will throw errors if you redefine it later.

There's a ton of other solutions. Lets move them into mattr_accessors or something...

Really doesn't make sense to use constants to define very dynamic data such as app paths. This is configurable stuff, not "set once and leave it".

jacquescrocker commented 14 years ago

initial fix to at least make it not blow up in Rails3. Still pretty lame however... http://github.com/railsjedi/jammit/commit/495c5426a0719ebb270fbbe2db6d4e5d94f2f70d

vandrijevik commented 14 years ago

Railsjedi,

What is the problem with the existing code in jammit.rb? Your commit does not contain any tests, so I am not sure what problem you are solving with it, or what this issue is about.

Can you provide some examples where the current code would not work?

jacquescrocker commented 14 years ago

The issue is loading order. Rails is defined when this lib is being run, however Rails.root is not defined.

I'll add tests and examples soon. This was just a quick patch so I could get it running in my project.

jacquescrocker commented 14 years ago

Also, I would suggest these values should be class level variables with accessors. Constants really seems to me like the wrong way to define these paths.

jashkenas commented 14 years ago

This is now merged to master. If you'd like to use something other than constants -- I don't much mind one way or the other -- feel free to submit a patch.

jacquescrocker commented 14 years ago

Cool, sounds good. I'll get these constants converted over to Class accessors. I think it'll make things much more flexible so people can change the path configurations after the plugin has already loaded