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

Memoize the result of Grape::Middleware::Base#response. #2192

Merged

Conversation

Jack12816
Copy link
Contributor

- What is it good for

When using the Grape::Middleware::Base#response method multiple times from within a custom middleware, the method generates a Rack::Response instance each (multiple) time.

Do not convert Rack::Response to Rack::Response was missing the memoization I think, because Don't create Grape::Request multiple times already added memoization for #request.

- What I did

While writing a simple middleware that should append a response header I hit an unexpected behavior while using the Grape::Middleware::Base#response helper. Example time:

class ResponseRoute < Grape::Middleware::Base
  # (1) Works with a Grape generated response,
  #     but not with a Rails generated response (looks like they aren't
  #     sharing the Rack::Utils::HeaderHash instance)
  def after
    response['X-Debug'] = 'true'
    response
  end

  # (2) Works with a Grape and Rails generated response 
  def after
    response.tap do |res|
      res['X-Debug'] = 'true'
    end
  end
end

After my fix, both versions will work as expected.

- A picture of a cute animal (not mandatory but encouraged)

images

Signed-off-by: Hermann Mayer <hermann.mayer92@gmail.com>
@Jack12816 Jack12816 force-pushed the memoized-response-middleware-helper branch from f6a393b to 1517aac Compare October 4, 2021 14:09
@dblock dblock merged commit 4edf54b into ruby-grape:master Oct 4, 2021
@Jack12816
Copy link
Contributor Author

@dblock could you add the hacktoberfest-accepted label? 🙃

@dblock
Copy link
Member

dblock commented Oct 5, 2021

@dblock could you add the hacktoberfest-accepted label? 🙃

What is this? :)

@Jack12816
Copy link
Contributor Author

It's https://hacktoberfest.digitalocean.com/ - a monthlong "hackathon" where the people are encouraged to contribute to open-source projects. :)

Yeah, and I thought you could add Grape as a participating project or label my PR's individually to count for me :)

@dblock
Copy link
Member

dblock commented Oct 5, 2021

Done!

@Jack12816
Copy link
Contributor Author

Awesome! Thank you ☺️

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.

2 participants