Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rsutphin opened this issue Sep 2, 2014 · 7 comments

Comments

@rsutphin
Copy link

rsutphin commented Sep 2, 2014

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
Copy link
Author

rsutphin commented Sep 2, 2014

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
Copy link
Member

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
Copy link
Author

rsutphin commented Sep 3, 2014

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
Copy link
Member

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
Copy link
Author

rsutphin commented Sep 3, 2014

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
Copy link
Member

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

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

@rsutphin
Copy link
Author

rsutphin commented Sep 5, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants