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

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

Closed
technodrome opened this issue Jul 24, 2018 · 15 comments
Labels

Comments

@technodrome
Copy link

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.

  1. 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: mislav/will_paginate#459

@ddnexus
Copy link
Owner

ddnexus commented Jul 24, 2018

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
Copy link
Author

Thank you @ddnexus, much appreciated!

@ddnexus
Copy link
Owner

ddnexus commented Jul 24, 2018

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
Copy link
Owner

ddnexus commented Jul 24, 2018

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
Copy link
Author

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
Copy link
Owner

ddnexus commented Jul 24, 2018

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
Copy link
Owner

ddnexus commented Jul 25, 2018

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
Copy link
Author

Thanks, I look forward to a solution.

@ddnexus
Copy link
Owner

ddnexus commented Jul 26, 2018

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
Copy link
Author

Excellent support, @ddnexus, as usual 👍 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
Copy link
Owner

ddnexus commented Jul 29, 2018

@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
Copy link
Author

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

@technodrome technodrome reopened this Aug 2, 2018
@technodrome
Copy link
Author

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
Copy link
Owner

ddnexus commented Aug 2, 2018

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

@ddnexus
Copy link
Owner

ddnexus commented Aug 3, 2018

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

@ddnexus ddnexus closed this as completed Aug 3, 2018
@ddnexus ddnexus added the fixed label Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants