apneadiving / Google-Maps-for-Rails

Enables easy Google map + overlays creation in Ruby apps
https://apneadiving.github.io/
MIT License
2.26k stars 382 forks source link

Use asset pipeline #59

Closed cameel closed 13 years ago

cameel commented 13 years ago

My patch makes the plugin use asset pipeline in Rails 3.1 while still being compatible with earlier versions. See the commit message for details.

I made the generator copy assets to public/assets if it detects Rails version >= 3.1. Maybe it would be even better to just display a message that nothing needs to be copied to keep people from breaking things if they use it anyway. This, however, would be a problem for those that have disabled the pipeline in their Rails config.

Tested with Rails 3.1.rc4 and Ruby 1.9.2p180

EDIT: It turns out that it's not fully compatible with Rails 3.0. Rails can serve assets directly from gem's public/ directory and moving them to app/assets breaks this. I'll need a better solution.

apneadiving commented 13 years ago

Thanks for your work! I'm busy these days but will integrate it soon :)

cameel commented 13 years ago

Thanks. But if you do, please make sure that my changes don't break serving public gmaps4rails.js as assets in Rails 3.0. As I remarked above (in EDIT), after creating this patch I have noticed that it probably will break.

I am thinking about fixing this with something along the lines of leaving the JavaScripts in public/ and somehow telling Rails that it should include them into the asset pipeline. Or maybe the other way around.

I'll probably take another stab at it during the weekend.

natebird commented 13 years ago

If you register gmaps4rails as a rails engine then the gem can provide the assets without any need for a generator. Rails 3.1 will look for assets inside of engines now.

For users on >= 3.0.x they can continue to use the current generator without issues.

cameel commented 13 years ago

Yeah. That's the idea behind this patch. But then I have noticed that in you could also just expect Rails to serve the assets directly from gem's public/ directory as long as you have it configured to serve static assets (as gmaps4rails README states). I don't know if anyone is actually relying on this, but I wouldn't like to break it for anyone that does.

Specifically this piece of code from lib/gmaps4rails.rb is responsible for that:

       initializer "static assets" do |app|
         app.middleware.use ::ActionDispatch::Static, "#{root}/public"
       end
natebird commented 13 years ago

If you really want to support the Rails 3.1 asset pipeline I think something is going to break for previous versions. Engines can serve assets from the /lib, /app/assets, or /public. Personally, I like the /app/assets as that is the convention going forward.

It would also be nice to remove the javascript from the views but that is another story.

cameel commented 13 years ago

I actually got it working without breaking anything. See my new pull request: https://github.com/apneadiving/Google-Maps-for-Rails/pull/64

I second the idea of removing javascript from views.

apneadiving commented 13 years ago

Interesting debate here.

1)

I agree the gem should support the asset pipeline in priority, even if it breaks the rest.

I still wonder if a mere symbolic link would be enough in the /public directory. I already use this with success in the dummy app to make jasmine find js files.

2)

Concerning the 'no js in views', I already tried to put the strict necessary. Do you have a better idea to optimize this?

cameel commented 13 years ago

1) As much as I agree with making Rails 3.1 support a priority, I wouldn't like to break existing functionality for Rails 3.0 users. Especially considering that it's still the most recent stable release.

I thought about symlinking too, but it has drawbacks - not every filesystem supports it. And I'm not sure if .gem packages do. If not, you would have to create them at installation time. It would be a hassle.

2) See my response in the thread for pull request #64