MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.

MIT License

3.72k stars 406 forks source link

Add support for Hotwire Turbo Drive #498

Closed ceritium closed 3 years ago

ceritium commented 3 years ago

It is related to this issue https://github.com/MiniProfiler/rack-mini-profiler/issues/482

I moved to the gem the @mildred's solution.

I tested it on a small demo app, as I show in the following video:

https://user-images.githubusercontent.com/16633/121650575-fa1ddc00-ca99-11eb-95eb-fc1b4fb0698d.mov

Any recommendation for better QA?

Dainii commented 3 years ago

Tested with rails (6.1.3.1) and turbo-rails (0.5.9), it seems to work quite nicely.

SamSaffron commented 3 years ago

This looks right to me @OsamaSayegh should we merge?

codecov-commenter commented 3 years ago

Codecov Report

Merging #498 (2826fc6) into master (a93060e) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #498   +/-   ##
=======================================
  Coverage   87.50%   87.50%           
=======================================
  Files          18       18           
  Lines        1257     1257           
=======================================
  Hits         1100     1100           
  Misses        157      157

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a93060e...2826fc6. Read the comment docs.

ceritium commented 3 years ago

Thanks for your feedback @OsamaSayegh. I like your approach. I just pushed a commit addressing your feedback and a small test.

Would you add more tests? Do you have any suggestions about variable naming? It's ok the place for enable_hotwire_turbo_drive_support on "config.rb" or would you move it to a different place?

OsamaSayegh commented 3 years ago

I think it looks great, thank you @ceritium!

Edit: One more thing; could you please document the new config in table the README file? https://github.com/MiniProfiler/rack-mini-profiler#configuration-options

ceritium commented 3 years ago

@OsamaSayegh I added the documentation.

Do you like the commits as they are? Would you squash them? I didn't found any policy about that.

OsamaSayegh commented 3 years ago

Perfect, thank you for all your work on this!

Github has a merge option that squashes everything together which I'm going to use now!