Add an explicit require for "active_record"

[hlascelles]

It is possible for super_diff to break with rails if required in a certain order.

You can demonstrate by running this file:

require "bundler/inline"

gemfile do
  source "https://rubygems.org"
  gem "rails"
  gem "super_diff"
end

require "rails"
require "super_diff"
require "super_diff/rails"

You get:

super_diff/lib/super_diff/rails.rb:1:in `<top (required)>'
super_diff/lib/super_diff/active_record.rb:39:in `<top (required)>'
super_diff/lib/super_diff/active_record/monkey_patches.rb:4:
    in `<top (required)>': uninitialized constant ActiveRecord (NameError)

This PR covers for that case.

[mcmire]

While I understand why this problem would be happening, it is not quite how the gem is intended to be used and should not occur for the average Rails app (and specifically once your Rails app boots/initializes). Is there something that you're trying to do in particular?

[hlascelles]

Fair question - in our case we have a number of gems which form the basis of engines, and we also have a gem who's job is to do nothing but prepare all of our test gems for the spec phase. It is that gem which is showing this issue - we can't set up that gem and point it at another gem which is an engine. Actually I also notice it is a problem for any rubygem engine that uses Combustion but doesn't initialize ActiveRecord (since it may not need it).

I do see that this may be less common....

A more compelling reason is that any gem must require its own dependencies, and it is violating rubygem specificaitons if it doesn't. In this case it is (almost!) certain that the load order will be right, but this change can handle rare(r) cases?

[mcmire]

@hlascelles Thanks for providing some more details, and that makes sense — it's true engines are rare but they are a feature of Rails and should be supported.

If there's a way to replicate the setup for an engine in the tests, then I'm happy to merge this fix. The bit of code you provided would work fine of course but I don't want to lose the use case where this originated so I'd rather get closer to that.

Currently the way the integration tests work is that a Rails application is generated on the fly, and so ideally we'd want to do the same thing for engines. The setup for that is a little complicated, so I'm inclined to take that step over rather than have you do this. Do you by chance have a repo that could serve as an example, or should I just try generating an engine using Combustion and starting with that?

[hlascelles]

I've added a few files that can demonstrate the issue with a combustion app. Just cd into the new directory and run bundle exec ruby combustion_example.rb and it will error. You can then change the line in the Gemfile to run it with the local version with the fix.

Thank you for looking into this, yes, it would be a great help if you added it to the tests the best way to fit everything else. Hopefully I've added enough for that to be trivial. 👍

[mcmire]

I took a look at your example and thought about this a bit more. If you have an engine that does not rely on ActiveRecord, I'm not sure that explicitly requiring ActiveRecord is the best answer, because then the gem is loading code that you don't want. Rather, since super_diff/rails is really a shortcut for requiring super_diff/active_support and super_diff/active_record, I wonder if it makes more sense to only require super_diff/active_record if ActiveRecord is available. That way if it is not, then the gem shouldn't ever try to reference ActiveRecord. I do think that we still need some tests for this, so I'm still working on integrating Combustion, but that's the direction I'm thinking of going in. What do you think about that? EDIT: Here is the branch I've been working on, starting from the most important bit.

[hlascelles]

Interesting points, I see what you mean. Yes, that would work. It would keep the "plug and play" simple. Go for it - and nice to see Combustion tests going in! 👍

But also... I suppose if the (good) intent is to make everything "just work" you could remove the need to super_diff/rails at all. It could do that if defined?(Rails) automatically. Possibly provide an escape hatch like an ENV SKIP_SUPER_DIFF_RAILS?

[mcmire]

Hey, sorry for the long delay in responding to this PR. I appreciate the test setup you provided here. I decided to keep your suggestion of allowing people to still use require "super_diff/rails" instead of forcing them to switch to require "super_diff/active_support" in these circumstances. In that case SuperDiff should do the right thing and only require the ActiveSupport portion of the gem if ActiveRecord has been loaded. Your changes are included in #188. Thank you!