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

Make helpers work without count #944

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

attilahorvath
Copy link

I've been working with somewhat larger data sets lately where SELECT COUNT queries would often lead to timeouts. I would like to use the without_count method to solve this issue, however I see no reason why I'd need to change the templates for this, I think the gem could handle this use case automatically by only rendering the prev/next link tags.

This PR enables the use of paginate and page_entries_info with non-count scopes by automatically degrading their features, disabling functionality that can't be used without knowing the result set size.

@yuki24
Copy link
Member

yuki24 commented Apr 9, 2018

@attilahorvath This is a very interesting idea. Thanks for bringing this up. I have some thoughts around how we would move forward about this.

What's on the table right now is a plan to change all kaminari scopes marked as "without_count" by default. In favor of that we could deprecate the #without_count method and reduce queries automatically for further optimization. This way developers would see less queries (depending on the use case, obviously) and get performance boost for free by just upgrading to the latest version. What this means is that anything that depends on the current design of the PaginatableWithoutCount module may not fit in well once we toggle the switch.

The other thing we can do is to add a paginate_without_count view helper method that shows as many elements as possible without making a COUNT query (which is what I've done for my TIL app), but I'm open to suggestions as to what interfaces are acceptable and what are not.

@attilahorvath
Copy link
Author

@yuki24 I think that's an excellent idea to make this the default. Individual developers can then pay the price if they want numbered links for specific pages.

My concern was the fact that I had to use a separate view template for non-count result sets that would be the same as our regular Kaminari template, just without these additional links. That would lead to code duplication and we would need to change both templates e.g. if we need to add a new class, which is easy to forget - it's not DRY. I think the gem should gracefully degrade the functionality by only showing prev/next links in this case which is what this PR intends to do.

As to how to invoke this behaviour after all scopes are non-count by default, I think we have a couple of options:

  • A separate paginate_without_count helper as you mentioned: paginate_without_count @users
  • A new optional flag for paginate: paginate @users, without_count: true
  • A model-level option similar to paginates_per:
class User < ActiveRecord::Base
  paginate_without_count
end

(For my specific use case that I'm working on right now, this last option would make the most sense.)

All of these have their pros and cons and of course we're not limited to just one of these, we could have e.g. both the separate helper and the model-level option as well.

Let me know what you think, I can offer my help with the implementation if you want.

@attilahorvath attilahorvath force-pushed the make-helpers-work-without-count branch from 448923a to bd5c378 Compare April 11, 2018 15:48
@attilahorvath
Copy link
Author

Fixed that one failing AR 4.1 test in the meantime.

@ruurd
Copy link
Contributor

ruurd commented Dec 20, 2020

Mind you that countless paginating is dependent on the way a view is sorted!!! It's not possible to just remember the last ID of an ActiveRecord::Base and then query for a page of records where id > last_seen_id. One must in that case determine what the sort order of the view is and use that to navigate to the next/prev pages. Hardly trivial at all.

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

Successfully merging this pull request may close these issues.

None yet

3 participants