Shopify/response_bank

Error pages other than 404s are uncacheable

jpqy opened this issue · 1 comments

jpqy commented

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.

Please feel free to submit a pull request.