firehoseio / firehose

Build realtime Ruby web applications. Created by the fine folks at Poll Everywhere.
http://firehose.io/
MIT License
727 stars 72 forks source link

Should sprockets be a regular gem dependency? #40

Closed andyshinn closed 8 years ago

andyshinn commented 10 years ago

At https://github.com/polleverywhere/firehose/blob/master/lib/firehose/assets.rb#L1 we require sprockets. But it is included only as a development dependency, which breaks deployment when running Bundler without development or test groups.

Should sprockets be a regular gem dependency? I'll submit a pull request for this trivial change if this is indeed the case.

bradgessler commented 10 years ago

This should only load if there's a reference to Firehose::Assets. Could you let me know where you're seeing the error so I can track down where that may be happening?

bf4 commented 10 years ago

A common idiom for this is

begin
  require 'sprockets'
rescue LoadError
  puts "continuuing without sprockets"
end

or to wrap that in a method

andyshinn commented 10 years ago

It is happening in another project requiring firehose and using the client to publish. The same error happens right out of the example client usage:

require 'firehose'
require 'json'
json = {'hello'=> 'world'}.to_json
firehose = Firehose::Client::Producer::Http.new('//127.0.0.1:7474')
firehose.publish(json).to("/my/messages/path")
$ bundle exec ruby test.rb
/Users/andy/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/firehose-1.2.12/lib/firehose/assets.rb:1:in `require': cannot load such file -- sprockets (LoadError)
    from /Users/andy/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/firehose-1.2.12/lib/firehose/assets.rb:1:in `<top (required)>'
    from /Users/andy/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/gems/firehose-1.2.12/lib/firehose.rb:22:in `<top (required)>'
    from test.rb:1:in `require'
    from test.rb:1:in `<main>'
bf4 commented 10 years ago

Well, looks like this is a circular dependency-type bug. Thanks to your stack trace

https://github.com/polleverywhere/firehose/blob/master/lib/firehose.rb#L22

# Detect if Sprockets is loaded. If it is, lets configure Firehose to use it!
Firehose::Assets::Sprockets.auto_detect

So, maybe it should check const_defined? or something?

bradgessler commented 10 years ago

I'd rather move this detection code into something like firehose/assets and require anything concerned about firehose JS assets to require "firehose/assets". This is more inline with splitting out firehose into several different gems.