Replace browserify-rails with rails-webpacker #1835
Conversation
| on roles(:app, :web), in: :sequence do | ||
| within release_path do | ||
| execute :npm, 'install' | ||
| execute :yarn, 'install' |
There was a problem hiding this comment.
Before merging, we'll want to work with devops to make sure yarn is available.
|
My general preference would be to run webpack directly as needed, but I think these are really solid changes. |
|
@tbaxter-18f: The downside of running webpack directly is we loose a few of the benefits of the assets pipeline, like the use of tag helpers to load thumbprinted assets. I've managed to work around that by bundling assets into the asset load path, but that gets messy and with larger bundles can mean we see more performance issues. |
|
Also, note that the code climate issues are related to files I moved, not files I've changed. I can fix the issues in them if we think it is a priority, but I'd prefer to keep the diff here small. |
|
I'm going to move this down to WIP. Looks like we'll need to move up to node v6 or v8 to use |
jmhooper
changed the title
jmhooper
force-pushed
the
jmhooper-use-webpacker
branch
from
19a9d49 to
34bd40b
Compare
jmhooper
changed the title
|
@jmhooper Wanna rebase with master? |
.circleci/config.yml
Outdated
| cp config/application.yml.example config/application.yml | ||
| cp certs/saml.crt.example certs/saml.crt | ||
| cp keys/saml.key.enc.example keys/saml.key.enc | ||
| bin/generate-example-keys | ||
| bundle exec rake db:setup --trace | ||
| bundle exec rake assets:precompile | ||
| bundle exec rake db:setup --trace |
There was a problem hiding this comment.
Looks like we already have this on line 65.
| == javascript_include_tag 'misc/i18n-strings' | ||
| - unless controller_name == 'home' | ||
| == javascript_include_tag 'application' | ||
| == javascript_include_tag 'i18n-strings' |
There was a problem hiding this comment.
should this be javascript_pack_tag?
There was a problem hiding this comment.
No, the i18n-strings use ERB so I've left it in the app directory for now. One thing to think about after we handle this may be to move that into a template like we do with the session timeout javascript.
| - unless controller_name == 'home' | ||
| == javascript_include_tag 'application' | ||
| == javascript_include_tag 'i18n-strings' | ||
| == javascript_pack_tag 'application' |
There was a problem hiding this comment.
We don't need the controller conditional anymore?
There was a problem hiding this comment.
I don't think a "home" controller exists anymore?
|
@monfresh: Addressed the things you mentioned and brought this up to date with master. Should be good to go now. Also note the synk issue is a false positive IIRC. It relates to using the dep in a Node.js app which we are not doing. |
monfresh
force-pushed
the
jmhooper-use-webpacker
branch
4 times, most recently
from
05034c1 to
a96a460
Compare
**Why**: rails-webpacker compiles the assets much faster than browserify-rails. With browserify-rails, our assets take so long to compile that the initial requests to the idp time out locally. I have not observed that with webpacker. Additionally, rails-webpacker is recommended by the maintainer of browserify-rails for projects that have already made the step towards using commonjs modules. One tradeoff of this is that rails-webpacker expects yarn, so we'll need to move from npm to yarn for this to work. Another tradeoff is that teaspoon does not appear to play nicely with rails-webpacker, so I removed it and replaced it with a bare-metal mocha.
monfresh
force-pushed
the
jmhooper-use-webpacker
branch
from
a96a460 to
0996a19
Compare
Why: rails-webpacker compiles the assets much faster than
browserify-rails. With browserify-rails, our assets take so long to
compile that the initial requests to the idp time out locally. I have
not observed that with webpacker.
Additionally, rails-webpacker is recommended by the maintainer of
browserify-rails for projects that have already made the step towards
using commonjs modules.
One tradeoff of this is that rails-webpacker expects yarn, so we'll need
to move from npm to yarn for this to work.
Another tradeoff is that teaspoon does not appear to play nicely with
rails-webpacker, so I removed it and replaced it with a bare-metal
mocha.
Hi! Before submitting your PR for review, and/or before merging it, please
go through the following checklist:
For DB changes, check for missing indexes, check to see if the changes
affect other apps (such as the dashboard), make sure the DB columns in the
various environments are properly populated, coordinate with devops, plan
migrations in separate steps.
For route changes, make sure GET requests don't change state or result in
destructive behavior. GET requests should only result in information being
read, not written.
For encryption changes, make sure it is compatible with data that was
encrypted with the old code.
Do not disable Rubocop or Reek offenses unless you are absolutely sure
they are false positives. If you're not sure how to fix the offense, please
ask a teammate.
When reading data, write tests for nil values, empty strings,
and invalid formats.
When calling
redirect_toin a controller, use_url, not_path.When adding user data to the session, use the
user_sessionhelperinstead of the
sessionhelper so the data does not persist beyond the user'ssession.