Uysim / pagy-cursor

Cursor pagination with pagy for Ruby On Rails
MIT License
126 stars 23 forks source link

Breaking change in 0.2.1 regarding return type from `pagy_cursor` (`ActiveRecord::Relation` -> `Array`) #28

Closed walro closed 2 years ago

walro commented 3 years ago

Hiya, I was upgrading to 0.2.1 from 0.2.0 and noticed that the return value of pagy_cursor changed from a ActiveRecord::Relation to a plain old Array. I tracked down the behavior change to #19 (diff link) where the subscript operator causes the query to be "eagerly" evaluated.

The following test case passes on 0.2.0 but not on 0.2.1:

diff --git a/spec/pagy_cursor/cursor_spec.rb b/spec/pagy_cursor/cursor_spec.rb
index d71bb53..074462d 100644
--- a/spec/pagy_cursor/cursor_spec.rb
+++ b/spec/pagy_cursor/cursor_spec.rb
@@ -62,6 +62,12 @@ RSpec.describe Pagy::Backend do
       expect(records.last.name).to eq("user100")
       expect(pagy.has_more?).to eq(false)
     end
+
+    it 'is lazy' do
+      _, records = backend.send(:pagy_cursor, User.all)
+
+      expect(records).to be_a(ActiveRecord::Relation)
+    end
   end

   context 'with ordered records' do

This was a bit surprising to us (or our code-base at least) and to me it seems like it might have made the gem a bit less useful as you might not always want your records loaded just yet. I understand the change in #19 was a good one, but maybe there is a way to still support the old "chainable relation" return type?

Uysim commented 2 years ago

@yunanhelmy Do you have any idea on this? Because what @walro mention here make sense that if we don't return in ActiveRecord::Relation, it will be less useful

yunanhelmy commented 2 years ago

@walro Thank you for pointing out.

@Uysim That was because the return is items[0..pagy.items-1], so it will become array. I will create PR to change the return not as array, but as ActiveRecord::Relation.

yunanhelmy commented 2 years ago

@walro @Uysim https://github.com/Uysim/pagy-cursor/pull/30 please let me know your feedback.

Uysim commented 2 years ago

@yunanhelmy I think the better solution here is to revert #19

Uysim commented 2 years ago

@walro and @yunanhelmy We have it fix at version 0.2.2