trailblazer / roar-rails

Use Roar's representers in Rails.
http://roar.apotomo.de
MIT License
235 stars 65 forks source link

require "roar/representer/json/hal" fails with roar-rails, but not with roar alone #76

Open rsutphin opened 10 years ago

rsutphin commented 10 years ago

I've got an API client written with roar-rails and embedded in a rails application. I'm trying to factor it out to its own gem so I can reuse it in other apps (including non-rails apps & scripts). It uses Roar::Representer::JSON::HAL for its representers, so in the separate gem I require the appropriate file to make that mixin available:

require "roar/representer/json/hal"

When I try to use the gem in the original app, I get an exception like this:

/Users/rsutphin/.rvm/gems/ruby-2.0.0-p481/gems/roar-rails-0.1.6/lib/roar/rails/hal.rb:3:in `<top (required)>': uninitialized constant Roar::Representer::JSON::HAL (NameError)
    from /Users/rsutphin/.rvm/gems/ruby-2.0.0-p481/gems/roar-0.12.9/lib/roar/representer/json/hal.rb:44:in `<module:JSON>'
    from /Users/rsutphin/.rvm/gems/ruby-2.0.0-p481/gems/roar-0.12.9/lib/roar/representer/json/hal.rb:4:in `<module:Representer>'
    from /Users/rsutphin/.rvm/gems/ruby-2.0.0-p481/gems/roar-0.12.9/lib/roar/representer/json/hal.rb:3:in `<top (required)>'

... originating on the line in the gem that requires "roar/representer/json/hal". I've reproduced this in a clean rails app: reproduction script.

In the rails app, if you just reference the constant Roar::Representer::JSON::HAL, everything works fine (which is why the client was working when it was embedded in the app). But this doesn't work with plain Roar, so I can't use this approach in my shared gem.

The problem appears to be a conflict caused by an autoload definition in roar-rails. This autoload tells ruby to require 'roar/rails/hal' to get Roar::Representer::JSON::HAL. When roar/representer/json/hal attempts to define module HAL, the autoload is triggered, resulting in roar/rails/hal being evaluated from within roar/representer/json/hal, before Roar::Representer::JSON::HAL is defined. Since roar/representer/json/hal is already being required, the require for it from within roar/rails/hal is ignored, and the subsequent attempt to access Roar::Representer::JSON::HAL fails.

I'm trying to figure out how to work around this. I wonder if the best solution might just to be to define the to_hal and from_hal aliases in Roar instead, then remove roar/rails/hal from the picture.

rsutphin commented 10 years ago

Here's how I'm working around this for now:

# in a Rails initializer

# XXX: work around https://github.com/apotonick/roar-rails/issues/76
Roar::Representer::JSON::HAL
require 'my_external_client_entry_point'
apotonick commented 10 years ago

Thanks for reporting this, Rhett. I will remove all autoloading in Roar 1.0 as it is completely unreliable and people can simply require files as they need. Will that fix your problem?

rsutphin commented 10 years ago

There doesn't seem to be any autoloading in the current version of Roar — do you mean roar-rails? Removing the autoload from roar-rails would prevent this issue for sure, though I'm not sure what you'd use to get the same effect. (Asking people to require a special file (like roar/rails/hal) for each representer type just because they're using roar-rails instead of plain roar sounds like a recipe for documentation confusion and support troubles.)

FWIW, I've found that autoloading works fine in straightforward situations (specifically where the desired constant is defined directly the file that's autoloaded) and seems to give a reasonable tradeoff between ease of use and loadtime/memory consumption. But it can definitely be tricky in edge situations like this one. (And I guess it's still not threadsafe on JRuby.)

apotonick commented 10 years ago

Ok. Hm... personally, I absolutely do not see the point about what's the problem of requiring a file explicitly like roar/json/hal or something. It's not that people need to require 200 files?!

I had several issues with autoloading (other peoples project + problem, of course ;), to an extend that I'd prefer to remove it.

rsutphin commented 10 years ago

Yeah, I don't see the problem with having to do something like

require 'roar/representer/json/hal'

... in order to be able to use Roar::Representer::JSON::HAL. That makes total sense to me. What this autoload seems to be doing, though, is adding new roar-rails-only behavior to the Roar::Representer::JSON::HAL module. You'd have to do something like

require 'roar/representer/json/hal'
require 'roar/rails/hal'

To get the same effect (in a Rails app) without the autoload. My guess is that this will confuse people since the representer itself would be the same whether you were using roar-rails or not.

apotonick commented 10 years ago

Ah, ok, thanks! Well, let's remove autoloading for roar-rails and roar?

Or maybe I just messed it up in roar-rails?

rsutphin commented 10 years ago

I wonder if there's a better way to get the same effect as the autoloading in roar-rails. I'll think about it.