-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Issue #782 : Ruby version upgraded to 2.7.2. #897
Conversation
Hi, @sharvy! I'm willing to spend some time looking into the test failures, that node error message. I can think of a couple of places to start adjusting for this, though I'm not 100% what's the best fix long-term. Do you have a good feel for this yourself? |
We are using node 12.x. But all the prebuilt images (Docker, CircleCI) have node 14.x with Ruby 2.7.2. One solution could be: Docker has ruby:2.7.2-alpine.3.12, but CircleCI doesn't have any prebuilt image with ruby 2.7.2 & node 14.x. So we can install nvm and swap node version in CircleCI.. I don't think that's a good solution for the long run. Another solution could be: Upgrading node to 14.x in our project. Best for the long run, according to my opinion. I went with the later solution, and everything is working perfectly. |
Another nice thing about doing this is that it would allow for easy use & packaging on NixOS, which packages only ruby 2.7.2 |
This looks great @sharvy 💥 |
@exbinary It looks good to me. 🙂 |
Hmm, so it deploys w/o error but loading any page generates the following error:
Looks like possibly a webpack configuration error? Could be something broke on the |
… since we upgraded version to 5+.
@exbinary I think I figured it out. |
@sharvy , appreciate you following up on this and good catch! Unfortunately that change itself didn't fix the issue. Good news is i believe i've found the two necessary fixes. I have a separate PR to upgrade just Feel free to cherry-pick (or reimplement) those two changes into your branch. I think this'll cause less merge conflicts than merging my PR first but i'm happy to do it that way if you prefer. Btw, for this and future troubleshooting, i was able to recreate the issue locally by running: rm -rf public/packs
NODE_ENV=production RAILS_ENV=production bin/rails assets:precompile At first, that showed the issue with |
@exbinary I have picked the changes from your PR. 🙂 |
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.
Looks great! 💥 🙏🏾
Why
To include security fix in Webrick and many other changes.
Resolves #782
How
bin/dev/test
, make sure all GreenPre-Merge Checklist