Repository navigation
Cover Rails 8.1 - #17
Merged
Merged
Conversation
The matrix stopped at 8.0. Adding 8.1 fails before any example runs: NameError: undefined local variable or method `stale_when_importmap_changes' for class ApplicationController rails new on 8.1 emits that macro into the generated ApplicationController. It comes from importmap-rails, and this dummy app runs on Sprockets because that is what Active Admin 3 needs, so the gem is absent and the app raises at boot. Sprockets is not the problem; the generated scaffold assuming importmap is. Skip JavaScript during generation so the macro never appears. The sibling active_admin_import uses the same fix, with the caveat that its specs run on rack_test. This suite drives real Chrome, so the relevant question is whether anything the specs need goes missing: it does not, because app/assets/javascripts/active_admin.js comes from active_admin:install rather than from rails new. Confirmed green on 7.1, 8.0 and 8.1. Active Admin 3.2 is excluded on 8.1 for the reason it is already excluded on 8.0: it predates Rails 8 support.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rails 8.1 has been out for a while (8.1.4 at time of writing) and the matrix did not cover it. Adding it fails immediately, and not because of anything in this gem:
rails newon 8.1 emits that macro into the generatedApplicationController. It comes fromimportmap-rails. This dummy app runs on Sprockets —sprockets-rails+sass-rails, because that is what Active Admin 3 needs — so the gem is absent and the app raises at boot, before a single example runs. Sprockets is fine; the generated scaffold is what assumes importmap.--skip-javascriptstops the macro being generated. This is the same fixactive_admin_importalready uses for its 8.1 leg.One difference worth checking rather than assuming: that repo's specs run on
rack_testand never execute JavaScript, so losing JS setup costs it nothing. This suite drives real Chrome through cuprite. Verified it is still fine —app/assets/javascripts/active_admin.jsis created byactive_admin:install, not byrails new, so--skip-javascriptdoes not touch the bundle the specs exercise.Checked locally before pushing:
Active Admin 3.2 is excluded on 8.1 for the same reason it is already excluded on 8.0 — it predates Rails 8 support.
Note on the versions being tested
Worth stating plainly, though this PR does not act on it. As of today:
So two of the three legs this matrix had were already past end of life, and the current series was untested. This PR fixes the second half. Dropping the EOL legs is a separate decision — 7.1 is the floor the gemspec declares, so dropping it from CI should come with raising that floor, which is a user-visible change and belongs in its own PR.