Do not lose the goldiloader with additional includes

[sobrinho]

By calling something like:

Company.all.each do |company|
  company.employees.includes(:person).to_a
end

We lose the goldiloader here because of the includes.

Although, the includes itself shouldn't cause any issues if we somehow simple merge it with the detected includes.

Another approach is to get rid of every single includes from the app if we rely entirely on goldiloader, but that's heavy compared to only adding the gem 🤷

[jturkel]

Thanks for filing this issue @sobrinho. Unfortunately calling includes (or any other methods from the Active Record query interface method like where) on an association creates a new query scope that is independent from the association so none of the existing Goldiloader code for detecting when to perform automatic eager loading on that association will work. I don't think we want to change this since it would break scenarios where you want to force the loading to happen at a particular point in the code (e.g. before deleting records from the underlying database) or change the query strategy (e.g. using joins).

I would recommend explicitly removing includes from your application where it's safe to do so (a tool like synvert could help automate this). Alternatively, you could monkey patch ActiveRecord::Associations::Association#includes to be a no-op but that could have unintended consequences.

[sobrinho]

Sounds good to me, thanks!