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

Error pages other than 404s are uncacheable #57

Open
jpqy opened this issue Jun 20, 2022 · 1 comment
Open

Error pages other than 404s are uncacheable #57

jpqy opened this issue Jun 20, 2022 · 1 comment

Comments

@jpqy
Copy link

jpqy commented Jun 20, 2022

Currently, response_bank only caches pages if the response code is a 200, 301, or 404:

if [200, 404, 301].include?(status) && env['cacheable.miss']

This prevents response_bank from being used to cache error pages other than 404s, even though caching other error pages (such as 429s) would bring the same performance benefits as caching 404 pages.

For example, only the 404 error page would be cached in this example:

class ErrorsController < ApplicationController
  def not_found
    response_cache(error_code: 404) do
      render(template: "errors/not_found", status: :not_found)
    end
  end

  def too_many_requests
    response_cache(error_code: 429) do
      render(template: "errors/too_many_requests", status: :too_many_requests)
    end
  end

  def internal_server_error
    response_cache(error_code: 500) do
      render(template: "errors/internal_server_error", status: :internal_server_error)
    end
  end
end

Caching responses with other error status codes that may depend on the state of the client (e.g. 429s depend on how many times the client has already hit the server) can be tricky, so I would suggest allowing response_bank to accept a configuration option that overrides the default array of cacheable response codes.

Please see https://github.com/Shopify/shopify-app-store/issues/17194 and https://github.com/Shopify/shopify-app-store/pull/17365/files#r901784102 and for more context on why App Store and Theme Store could use this change.

@rafaelfranca
Copy link
Member

Please feel free to submit a pull request.

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

No branches or pull requests

2 participants