-
Notifications
You must be signed in to change notification settings - Fork 120
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
Rails 7 Update #5874
Rails 7 Update #5874
Conversation
KludgeKML
commented
Apr 13, 2022
•
edited
Loading
edited
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.
This looks OK to me - a couple of questions but not blockers.
I notice the heroku app has failed to deploy but think that is a Heroku issue. Has it been deployed/tested on Integration?
@@ -2,7 +2,8 @@ | |||
"srcDir": "public/assets", | |||
"srcFiles": [ | |||
"**/*.js", | |||
"!express/**/*.js" | |||
"!express/**/*.js", | |||
"!**/visualise-*.js" |
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.
Do we have a note or card somewhere to remind us to add this back when possible?
tags: TAGS, | ||
comment: { body: body }, | ||
}) | ||
self.class.client.zendesk_client.tickets.create!(payload) |
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.
this seems to be undoing a change from the Fix deprecation notices
commit. Is it possible to remove the change from that commit?
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.
It's... not quite undoing it (you'll see that the end result is slightly different), but yeah, I might be able to tinker with the commits to go straight from the original version to this, let me see if I can do that.
including visualise.js causes the test suite to fail even though all the tests pass because it relies on adjacencyList (not present as the suite loads because in production it's provided by the HTML). Since we're not testing this yet, remove for the moment.
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.
Excuse my drive by review - I get email notifications as I think I already commented. I thought I'd offer some suggestions having worked on a few JS testing and sprockets things.
//= link accessible-autocomplete/dist/accessible-autocomplete.min.css | ||
//= link application.css | ||
//= link print.css | ||
//= link joint.css | ||
//= link visualise.css | ||
//= link accessible-autocomplete/dist/accessible-autocomplete.min.js | ||
//= link application.js | ||
//= link dagre.js | ||
//= link joint.layout.DirectedGraph.js | ||
//= link joint.js | ||
//= link joint.patch.js | ||
//= link test-dependencies.js | ||
//= link visualise.js |
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.
I imagine you can simplify this a little:
//= link_directory ./stylesheets
//= link joint.css
//= link application.js
//= link dagre.js
//= link joint.layout.DirectedGraph.js
//= link joint.js
//= link joint.patch.js
//= link test-dependencies.js
//= link visualise.js
You should also remove the assets_precompile stuff from
smart-answers/config/initializers/assets.rb
Lines 14 to 18 in d3fc1b1
Rails.application.config.assets.precompile += %w[ | |
test-dependencies.js | |
accessible-autocomplete/dist/accessible-autocomplete.min.js | |
accessible-autocomplete/dist/accessible-autocomplete.min.css | |
] |
smart-answers/config/application.rb
Lines 63 to 72 in d3fc1b1
config.assets.precompile += %w[ | |
joint.patch.js | |
joint.js | |
joint.layout.DirectedGraph.js | |
joint.css | |
print.css | |
dagre.js | |
visualise.js | |
visualise.css | |
] |
A couple of suggestions that could make this part of the update a little easier are:
- Do the sprockets update as part of the dependabot PR: Bump govuk_publishing_components from 28.9.2 to 29.5.0 #5879 (which updates Sprockets) so you can separate it out from the Rails 7 stuff
-
- Perhaps use asset-pipeline to require the various joint.* js files from visualise.js, I don't think they're used anywhere else.
@@ -7,7 +7,10 @@ | |||
"scripts": { | |||
"lint": "yarn run lint:js && yarn run lint:scss", | |||
"lint:js": "standardx 'app/assets/javascripts/**/*.js' 'spec/javascripts/**/*.js'", | |||
"lint:scss": "stylelint app/assets/stylesheets/" | |||
"lint:scss": "stylelint app/assets/stylesheets/", | |||
"jasmine:prepare": "bundle exec rake assets:precompile", |
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.
"jasmine:prepare": "bundle exec rake assets:precompile", | |
"jasmine:prepare": "RAILS_ENV=test bundle exec rails assets:clobber assets:precompile",", |
This brings it into convention with our other apps. The clobber is needed otherwise the same JS files will get included twice if a JS file is changed :(
"lint:scss": "stylelint app/assets/stylesheets/" | ||
"lint:scss": "stylelint app/assets/stylesheets/", | ||
"jasmine:prepare": "bundle exec rake assets:precompile", | ||
"jasmine:ci": "yarn run jasmine:prepare && PATH=~/.webdrivers:$PATH yarn run jasmine-browser-runner runSpecs", |
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.
"jasmine:ci": "yarn run jasmine:prepare && PATH=~/.webdrivers:$PATH yarn run jasmine-browser-runner runSpecs", | |
"jasmine:ci": "yarn run jasmine:prepare && yarn run jasmine-browser-runner runSpecs", |
This suggestion is for consistency with our other apps: https://github.com/alphagov/content-publisher/blob/6511019bc2313b1cea3f1fcd7f5e77aef6913368/package.json#L12
This app doesn't have the webdrivers gem (it was removed from GOV.UK test) alphagov/govuk_test#43
"srcFiles": [ | ||
"**/*.js", | ||
"!express/**/*.js", | ||
"!**/visualise-*.js" |
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.
I reckon you could change this to be:
"srcFiles": [ | |
"**/*.js", | |
"!express/**/*.js", | |
"!**/visualise-*.js" | |
"srcFiles": [ | |
"test-dependencies-*.js", | |
"application-*.js" |
As I think they are the only JS files involved in testing (all the visualise and joint stuff seem to be separate libraries for one distinct page)
Superceded by: #5892 |