sstephenson / ruby-yui-compressor

A Ruby interface to YUI Compressor for minifying JavaScript and CSS assets.
217 stars 57 forks source link

Failing early if java is not installed #40

Closed betesh closed 10 years ago

betesh commented 10 years ago

For #38

I added .travis.yml since @stevecrozz insisted in #38 that this work in all currently supported environments. Since Travis-CI doesn't support widonws, and can only support Mac OS or Linux on a given branch, I opted to only implement this feature for Linux. Mac OS and Windows users are no worse off than they were before.

stevecrozz- commented 10 years ago

Failing earlier makes some sense, but I'd like to know more about your motivation so I can understand what you're looking for. In your app is there potentially a long period of time between creating the YUI::Compressor object and calling #compress? By 'fail early' do you mean you want to save yourself the time in between #new and #compress in cases where Java is not available?

betesh commented 10 years ago

Yes, that is exactly what I mean. Perhaps there is not such a long period of time--maybe only a few milliseconds--but it is still good style to catch problems at the earliest possible point in the code--it gives us much more flexibility to output a clear error message.

stevecrozz commented 10 years ago

I think you're right that we can do something better. Here are my thoughts so far. We could use 'which' like you suggested. Although there are some limitations discussed nicely here: http://stackoverflow.com/questions/592620/how-to-check-if-a-program-exists-from-a-bash-script

Some of these arguments against using which only apply to shell scripts, but there's a few others that are worth considering in our case.

Here's another thought. If we're considering going as far as to fork a subprocess (which #{java_path}), why not execute something a little more authoritative like #{java_path} -version. That would make sure Java actually can run and it should work across all platforms.

And if we were to go that far, we might also want to run the yui compressor jar file as well #{java_path} -jar lib/yuicompressor-2.4.8.jar --version. That could assure us that java is available, and the jre we have is compatible with yui compressor, but this approach takes a bit longer to run. On my system its about 250ms for both checks. That's not bad, but its probably something that should be available on an opt-in basis.

For instance, maybe as a user you could do something like this:

compressor = YUI::JavaScriptCompressor.new()
compressor.sanity_check!
# YUI::Compressor::RuntimeError: Java is not installed or not on your PATH

or the opt-in could be rolled up into the constructor:

compressor = YUI::JavaScriptCompressor.new(:sanity_check => true)
# YUI::Compressor::RuntimeError: Java is not installed or not on your PATH

Any thoughts?

betesh commented 10 years ago

The jar file exists if you've installed the gem correctly. If the installed gem was altered so poorly that the jar is now missing, why would I trust that the Ruby code hasn't been altered too--i.e. the user has probably monkey-patched his way around any sanity checks that would force the jar to be there. There's nothing I can do to stop that so why check for it?

Lastly, if java is missing, you can't continue, so why give the user a way to opt out of the precondition check?

@sstephenson I'd really like you to chime in with you opinion here.

@stevecrozz What I learned from the SO link you reference is to use command -v instead of which (I'll change that if/when @sstephenson indicates he plans to accept this PR), but I disagree with all your other suggestions.

stevecrozz commented 10 years ago

I like some of the changes intended to modernize this gem, but I'm going to close this because it introduces platform-specific behavior (the check only works on linux) and by default relies on an extra sub-process.

Potentially raising an exception on initialize is also a minor backwards incompatible change. It's possible that could bite someone who's using this gem in a strange way. For instance, someone might initialize the compressor without compressing something and they'd get this new RuntimeError where they didn't previously. New default behavior is just not something I want to put into this old gem.