exAspArk / batch-loader

:zap: Powerful tool for avoiding N+1 DB or HTTP queries
https://engineering.universe.com/batching-a-powerful-way-to-solve-n-1-queries-every-rubyist-should-know-24e20c6e7b94
MIT License
1.04k stars 52 forks source link

Additional Ruby 3 fixes relating to kwarg changes #80

Closed jamesbrooks closed 10 months ago

jamesbrooks commented 3 years ago

Fixes some failures when using Ruby 3 in situations where it appears a few layers of delegation occur inside other libraries (in this case I was having failures using batch-loader in conjunction with view_component or dry-initializer).

This change passes the current tests on both ruby 3 and previous ruby versions. Sorry for not being able to include a test directly to replicate this issue I haven't dug into exactly what those other gems are doing to trigger this issue but the changes here should be innocent enough to not cause too much alarm I hope! :)

jeffmeyers commented 1 year ago

Hi, any movement on merging this into a release? Anything I can do to help? We see the same deprecation warnings and are preparing to upgrade to Ruby 3.

tony-pizza commented 1 year ago

Bump! batch-loader is not safe to use in Ruby 3 without these changes!

Here's a patch to add a spec if that's the blocker here. It fails in Ruby 3 on master and passes in this branch.

Thanks @jamesbrooks!

diff --git i/spec/batch_loader_spec.rb w/spec/batch_loader_spec.rb
index 7755c96..ab36b6b 100644
--- i/spec/batch_loader_spec.rb
+++ w/spec/batch_loader_spec.rb
@@ -85,6 +85,19 @@ RSpec.describe BatchLoader do

       expect(result).to eq([1, 2])
     end
+
+    it 'forwards arguments' do
+      user = User.save(id: 1)
+      post = Post.new(user_id: user.id)
+
+      batch_loader = post.user_lazy
+
+      expect(
+        batch_loader.method_with_arg_kwarg_and_block(1, b: 2) do |a, b|
+          a + b
+        end
+      ).to eq(3)
+    end
   end

   context 'with custom key' do
diff --git i/spec/fixtures/models.rb w/spec/fixtures/models.rb
index 6a45064..5391c8f 100644
--- i/spec/fixtures/models.rb
+++ w/spec/fixtures/models.rb
@@ -81,6 +81,11 @@ class User
     other.is_a?(User) && id == other.id
   end

+  # Used to test argument forwarding
+  def method_with_arg_kwarg_and_block(a, b:, &block)
+    block.call(a, b)
+  end
+
   private

   def some_private_method
franee commented 10 months ago

Bump! @exAspArk can you merge this please? I confirm this fixes my issue with ruby 3.0.6 and rails 6.1.7.6.

exAspArk commented 10 months ago

@jamesbrooks @tony-pizza thank you for submitting this PR and sharing your code! @jeffmeyers @franee thank you for bumping this and sorry for the huge delay!

💛

I just released these changes in version 2.0.2