rstacruz / sinatra-assetpack

Package your assets transparently in Sinatra.
http://ricostacruz.com/sinatra-assetpack/
MIT License
542 stars 97 forks source link

Optionally only build packages in production #169

Closed leoc closed 10 years ago

leoc commented 10 years ago

Precompiling for production should not compile all separate files. When setting :production_packages_only to true in the Sinatra app, the Builder will not build files if RACK_ENV is production.

I was not quite sure how to access the sinatra settings from the builder. The suggested implementation works, but I would love your suggestion on this. :-)

Cheers!

leoc commented 10 years ago

Oh dang! Forget to unstage them when amending.

I force pushed the amended commit

j15e commented 10 years ago

I think you could access settings with @app.settings.environment for exemple.

The README.md should be updated to explain this new option.

leoc commented 10 years ago

You are right. I changed that and used @app.settings for the new setting aswell to keep it consistent.

j15e commented 10 years ago

Awesome! I think it would be even better if the option was set along the others in the assets configuration block.

You could add this in the Option class around here :

https://github.com/rstacruz/sinatra-assetpack/blob/master/lib/sinatra/assetpack/options.rb#L174

And then use ex. @app.assets.production_packages_only.

I think the option should be renamed to be clearly linked to build task like build_packages_only and/or maybe the very best think would be to have separate rake task and avoid having to set an option for build in app.rb, for exemple :

rake assetpack:build          # Build packages and files
rake assetpack:build:packages # Build packages only
rake assetpack:build:files    # Build files only

(also, maybe we should rename build -> precompile to match sprocket terms, but that is an other story)

leoc commented 10 years ago

I totally agree, this was just a quick fix while not having to dive too deep into sinatra and this gem.

I would like to implement your latter suggestions tomorrow. :-) Thinking about it I, too, think that the separate build functions and rake tasks are the cleanest solution. This should not depend on a variable to be set inside the app configuration.

I don´t know yet, how the production sinatra-assetpack serves the files. Does it use the builder aswell? Nevermind the builder uses the rack server to get the compiled files ...

leoc commented 10 years ago

I added two new rake tasks under the assetpack:precompile namespace and left an assetpack:build alias with deprecation warning to keep backwards compatibility for existing scripts. Cheers.

j15e commented 10 years ago

Awesome! Just one last thing : since the README is already very very long, I would remove the information about deprecation of build in the README (the warning in the rake alias is enough).

Much kudos for your work

j15e commented 10 years ago

I will add a deprecation notice to UPGRADING for user installing new version when I push next gem release.

leoc commented 10 years ago

Removed obsolete README text and amended my commit.

j15e commented 10 years ago

Thanks, if you ever come by Quebec City I'll owe you a beer!

In the meantime, a thumb-up gif.

selfie-0