getappmap / appmap-ruby

AppMap client agent for Ruby
https://appland.org
Other
100 stars 13 forks source link

Hook Rack in Rails #326

Closed dividedmind closed 1 year ago

dividedmind commented 1 year ago

Hook requests in Rails on the Rack level in addition to ActionController. This allows intercepting any changes and responses made by the middleware. We're still hooking ActionController, because controller tests don't use Rack, but the ActionController hook is skipped if the rack hook has been activated on the request.

Fixes #323 (I believe)

dividedmind commented 1 year ago

Note something breaks the build, I'm trying to figure it out, I've switched it to draft.

dividedmind commented 1 year ago

Code looks good to me.

It seems worth installing it in an app that uses Devise, though, and verifying that the user's problem is actually fixed.

This seems like a possibility: https://github.com/ManickYoj/rails7-devise-template, or maybe one of these would do: https://henrytabima.github.io/rails-setup/docs/devise/example-applications .

I did (I created my own test app from scratch) and it does seem to solve the issue. I was reluctant to add it to tests so as not to complicate them unnecessarily.

apotterri commented 1 year ago

Code looks good to me. It seems worth installing it in an app that uses Devise, though, and verifying that the user's problem is actually fixed. This seems like a possibility: https://github.com/ManickYoj/rails7-devise-template, or maybe one of these would do: https://henrytabima.github.io/rails-setup/docs/devise/example-applications .

I did (I created my own test app from scratch) and it does seem to solve the issue. I was reluctant to add it to tests so as not to complicate them unnecessarily.

Ok, that's fine. Your initial comment didn't seem all that confident, so I just wanted to make sure.

Thanks!

kgilpin commented 1 year ago

:tada: This PR is included in version 0.99.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: