MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.
MIT License
3.69k stars 401 forks source link

Require Rails 6.0+ #548

Closed gmcgibbon closed 1 year ago

gmcgibbon commented 1 year ago

Use bundler to require Rails 6.0 or greater as a peer dependency. We should use Rails 6+ here because that is what's currently supported in the Ruby on Rails maintenance policy.

Also looks like https://github.com/MiniProfiler/rack-mini-profiler/pull/539 can be merged with this change. Rails 2.x support will be dropped. If a developer wants to reference this for an older version of the profiler, they can switch to the version tag.

For reference, this approach is inspired by how we manage peer deps in Rails: https://github.com/rails/rails/blob/dcd8f37bc63d46f28aa55e6891b03a0b0b73e497/activerecord/lib/active_record/connection_adapters/mysql2_adapter.rb#L6

codecov-commenter commented 1 year ago

Codecov Report

Merging #548 (0c77ab0) into master (edbcad3) will increase coverage by 0.06%. The diff coverage is 100.00%.

:exclamation: Current head 0c77ab0 differs from pull request most recent head c692831. Consider uploading reports for the commit c692831 to get more accurate results

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
+ Coverage   87.39%   87.46%   +0.06%     
==========================================
  Files          18       18              
  Lines        1269     1268       -1     
==========================================
  Hits         1109     1109              
+ Misses        160      159       -1     
Impacted Files Coverage Δ
lib/rack-mini-profiler.rb 100.00% <100.00%> (+3.44%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

nateberkopec commented 1 year ago

I'm usually not in favor of changing version requirements based on EOL policies, but rather on actual support for the versions in question.

Why should we drop support for old Rails versions that still work? RMP is really the only performance tool available for old Rails applications, why should we lock those apps out of newer versions of RMP?

gmcgibbon commented 1 year ago

I'm usually not in favor of changing version requirements based on EOL policies, but rather on actual support for the versions in question.

That's fair, I agree under most circumstances we should only drop support for Ruby/Rails versions when we have a real need to do so (via a code change etc.).

Why should we drop support for old Rails versions that still work? RMP is really the only performance tool available for old Rails applications, why should we lock those apps out of newer versions of RMP?

I think what I was trying to clarify with Rack Mini Profiler is that it should support some minimum version of Rails. Right now, we seem to specify a minimum of 3.0 (because the Railtie doesn't get included below that). We also don't have any CI integration tests to make sure it works with any Rails version, so I think it is unclear what this gem supports and what it doesn't. Clarifying expectations here would be helpful for maintainers of the library, and for uses of the library.

As an aside, Rails developers who are still using Rails <6 don't have security patch support, and are running vulnerable version web applications that need to be updated anyway. To me, this is just more incentive to upgrade, but I don't feel strongly about this. Happy to close, or lower the version requirement. Let me know what you think.

nateberkopec commented 1 year ago

Right now, we seem to specify a minimum of 3.0 (because the Railtie doesn't get included below that).

I think it makes sense to merge that hard requirement at 3.0 today (because as you say, we literally know the gem doesnt work for 2.0), and we can look at adding specs in the future. If the specs tell us our support for 3.0/4.0 etc is broken, we can make the decision at that point to change Rails version support in the gemspec.