ddnexus / pagy

🏆 The Best Pagination Ruby Gem 🥇
https://ddnexus.github.io/pagy
MIT License
4.6k stars 409 forks source link

Duplicate Rails fragment caching and link to first page without param[:page] #72

Closed technodrome closed 6 years ago

technodrome commented 6 years ago

I have pagination active on my homepage for a gallery. It shows different images for different user groups (logged in, admin, editor, ...).

When I click homepage logo, the URL is: localhost:3000/

However, pagination link for the first paginated page shows localhost:3000/?page=1

This raises two problems: 1) pages are identical in content, but seen as two different pages by search engines. We should use a canonical one. Current result for the first paginated page: localhost:3000/?page=1 Expected result for the first paginated page only: localhost:3000/

Basically, pagination should start inserting ?page= into URL from number 2 upwards. This can be seen on many websites with video galleries on the homepage.

2) I am using Rails fragment caching and inserting page number into cache_key to uniquely indentify cache fragments. This is a problem as there is no page number when clicking canonical URLlocalhost:3000 but there is one for localhost:3000/?page=1. Again, two pages, same content = two different cache fragments are generated for the same content. I got bitten by this problem where cache was expired for one of these fragments, but not for another one - users see different images.

I got around this by implementing a helper. Downside: hardcoded @pagy instance which is a problem when someone has several paginations active on a single page. It checks whether @pagy is defined on that page and whether it has params[:page] in the URL. If @pagy is defined, but no params[:page] is in the URL, it assumes a homepage and inserts default page number 1 into cache_key fragment name.

Helper

  def cache_with_pagy_support(*args, &block)
    # pagy is not defined, we do not have any pagination, no need to slip params[:page] or page 1 into cache_key
    return cache(*args, &block) unless @pagy

    # @pagy defined, we have pagination, need to check whether page number is defined or not
    # if page is not defined, insert default page: 1 into cache_key

    # block will get executed only if @pagy is defined and params[:page] is not defined
    # we are probably at the homepage, so insert default page: 1 into cache_key
    p = params.fetch(:page) do
      args[0].push(1) # modify args, slip in 1 as page:1
      return cache(*args, &block) 
    end

    # @pagy defined, params[:page] was found, so enter that specific page number as cache_key
    args[0].push(p)
    cache(*args, &block)
  end

View

# current_role inserts 'admin', 'member', ... into cache_key name
# page number is being inserted automatically by cache_with_pagy_support itself
- cache_with_pagy_support [:images, current_role] do
  .card-deck
    = render partial: 'image', collection: @images, cached: true

This works and inserts correct keys when visiting homepage without any page param in the URL: Write fragment views/images/member/1/ebb46c39e960e982acb853421cf145e4 (0.1ms)

Subsequent visits to either canonical homepage url without page param or with page=1 param read correct cache fragment: Read fragment views/images/member/1/ebb46c39e960e982acb853421cf145e4 (0.1ms)

However, I don't like such solutions as they assume too much about something which may change. Is there any way to get links without page=1 for the first page of the pagination sitewide?

will_paginate had something similar here: https://github.com/mislav/will_paginate/issues/459

ddnexus commented 6 years ago

hi @technodrome, I received the same request (without all that detailed explanation) on the gitter chat, and I turn it down because - at that time - I didn't see any benefit while I saw a slight performance loss. I will take a look at it and I will see what is the best possibility to address it.

technodrome commented 6 years ago

Thank you @ddnexus, much appreciated!

ddnexus commented 6 years ago

OK... here is what you can do. Just override the pagy_url_for in your helper:

    def pagy_url_for(page, pagy)
      p_vars = pagy.vars; params = request.GET
      params = params.merge(p_vars[:page_param] => page) unless page == 1
      params.merge!(p_vars[:params])
      "#{request.path}?#{Rack::Utils.build_nested_query(pagy_get_params(params))}#{p_vars[:anchor]}"
    end

That will remove the page=1 bit.

ddnexus commented 6 years ago

Another easier option would be using the pagy_get_params:

def pagy_get_params(params)
  params.delete(:page) if params[:page] == 1
  params
end

Probably better than the previous suggestion. (I didn't actually try it... you may have to care about the value as string or integer)

technodrome commented 6 years ago

After I put the second snipped to application_helper.rb, it removed page param from all pagination links. Now they look like: http://localhost:3000/?, all of them. I will try first one tonight.

ddnexus commented 6 years ago

It was missing the params as the return value... I edited it a few seconds after I posted it, but you probably got it from the original first email :)

ddnexus commented 6 years ago

Hmmm... sorry, I just remembered that the super fast proc that Pagy uses to render the links calls that methods only once and without the real page number, hence the methods that I suggested cannot work. D'oh :) Back to square one. I will think about the best solution for this.

technodrome commented 6 years ago

Thanks, I look forward to a solution.

ddnexus commented 6 years ago

After a lot of thinking, it looks like the simplest and more efficient way to strip the page=1 param is using a regexp on the output of the pagy_pav* helpers. For example (for a default "page" :page_param):

<%== pagy_nav(@pagy).gsub(/((?:[?&])page=1(?=")|\bpage=1&)/, '') %>

That is fast and safe and consistent with the fact that Pagy deals mostly with strings.

I tried changing the code in an extra to skip it at generation time, but it was ugly and it needs to remove part of the code that makes Pagy ~29x faster than Kaminari.

The regexp is the best way IMO. A possible API improvement could be an extra that encapsulates the regex as a Pagy instance method so it could be used more easily (regardless the :page_param). For example:

<%== pagy_nav(@pagy).gsub(@pagy.page_param_1_regexp, '') %>

However it doesn't work with the compact helpers because the url is built in a javascript function... I am looking to integrate the same solution with all the nav extras.

technodrome commented 6 years ago

Excellent support, @ddnexus, as usual :+1: Thank you for fast reply and working solution! I extracted the code into the applilcation_helper and indeed it works very well. Now the site has uniform pagination.

I will test it with Rails caching and report back in case I discover some irregularity. I am quite curious what you will come up with in the future.

Thank you again.

ddnexus commented 6 years ago

@technodrome Just pushed the v0.15.0 to rubygem. You should just require the trim extra in the initializer and it will work with any :page_param and any helper and template.

technodrome commented 6 years ago

@ddnexus amazing support again :+1: I will try the new version out as soon as possible. Thank you for your effort.

technodrome commented 6 years ago

Installed the newest version, page 1 does show the page=1 but only when I am on the first page. When I visit page 2 and hover over pagination link to page 1, in that case I do not see page=1 in the href. Please see the short GIF, should be clearer that way. firstpage_links

ddnexus commented 6 years ago

OK, that's only when the current page is the first page. I will take a look. Thanks.

ddnexus commented 6 years ago

Fixed in dev... will be pushed to rubygem soon. Thanks.