-
Notifications
You must be signed in to change notification settings - Fork 440
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
Add rails 8 support #2167
Add rails 8 support #2167
Conversation
@joelhawksley sorry I only see you also had a pr for this now, I have however fixed the issues that caused your pr to fail, if you don't mind taking a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @reeganviljoen, thanks for putting this PR together!
Does Rails 8 no longer include sprockets? If not, can we switch to propshaft instead? I think that's what most Rails 8 apps will be using, and it'd be nice to test against the most common configuration.
@@ -1,3 +1,3 @@ | |||
# frozen_string_literal: true | |||
|
|||
Rails.application.config.assets.precompile += %w[admin.css] | |||
if Rails.version.to_f <= 7.2 then Rails.application.config.assets.precompile += %w[admin.css] end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to change? EDIT: do we still need admin.css? I don't know what it's used for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but propshaft auto includes it
Co-authored-by: Cameron Dutro <[email protected]>
@camertron I have added propshaft at last, it does however come with less features by design, like for instance I can not set a custom host as far as I know so those tests are omitted on new rails versions |
@camertron @Spone @joelhawksley @BlakeWilliams anyone had a chance to review this, it is quite important |
Thanks @joelhawksley for the additions |
* add rails 8 appraisal * add rails 8 ci * fix lint * updatre gemfile * fix lint * fix specs * fix specx=cs again * fix specs * fix specs * f*** these specs already * fix lint * fix benchmarks * f*** these specs already * fix lint * fis spcs * fix ci * Update test/sandbox/test/rendering_test.rb Co-authored-by: Cameron Dutro <[email protected]> * fix ci * fix lint * fix lint * try fix benchmark ci * Apply suggestions from code review * Update test/sandbox/test/rendering_test.rb --------- Co-authored-by: Cameron Dutro <[email protected]> Co-authored-by: Joel Hawksley <[email protected]>
* add rails 8 appraisal * add rails 8 ci * fix lint * updatre gemfile * fix lint * fix specs * fix specx=cs again * fix specs * fix specs * f*** these specs already * fix lint * fix benchmarks * f*** these specs already * fix lint * fis spcs * fix ci * Update test/sandbox/test/rendering_test.rb Co-authored-by: Cameron Dutro <[email protected]> * fix ci * fix lint * fix lint * try fix benchmark ci * Apply suggestions from code review * Update test/sandbox/test/rendering_test.rb --------- Co-authored-by: Cameron Dutro <[email protected]> Co-authored-by: Joel Hawksley <[email protected]>
What are you trying to accomplish?
Add rails 8 appraisal and ci to matrix and move sprockets to be depended on for older versions