juliocesar / rack-pagespeed

Page optimizations done at the Rack level
http://rack-pagespeed.heroku.com
245 stars 22 forks source link

Don't explicitly require dalli and memcached. #35

Open envygeeks opened 11 years ago

envygeeks commented 11 years ago

I don't think it should be required since feasibly a user can use disk. For us we use a custom version of Dalli and Redis gem so your explicit require just forces us to modify your gem which is something I feel we shouldn't have to do just for a dependably change.

juliocesar commented 11 years ago

Here's the problem: if it's out of the Gemfile, it won't Just Work work for Heroku. Both Dalli and Redis are only actually required when you try to use those stores, but if I leave it out of the manifest, they won't get installed unless the user knows it needs to add it themselves.

There'd be quite a few more tickets if I left it out than in it. Sorry to hear it creates this problem for you, but due to how Ruby package managent works, it's the best way.

P.S.: happy to accept a patch if you can think of a solution that addresses those problems.

envygeeks commented 11 years ago

You could handle it the way it's handled at Pry, in that if they have a certain feature enabled via config and it doesn't need to be a pure dependency... if you are unable to load the gem and rescue out of the LoadError raise and tell them to install the Gem and note that in the Readme also, for certain features they will have to install the Gems themselves.

This also works on Heroku since it will show the raise (and you can detect the Linux or OS X -- actually just bash or zsh period and output it in red so they don't miss it amongst the Heroku messages.)

juliocesar commented 11 years ago

Ok, fact is, the redis gem isn't being required outside of the :test group anyway, so it doesn't make sense that dalli is.

Removed, bumped, and pushed to Rubygems. Thanks.