-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Depend on a specific version of Node.js #5158
Conversation
e560213
to
138d15c
Compare
53b511e
to
8ba0db0
Compare
3fad249
to
35884f9
Compare
dec4b9c
to
ed70d57
Compare
ed70d57
to
aba056d
Compare
aba056d
to
b252ae1
Compare
Hi @javierm, I've been testing deployments from this implementation (with It worked well in all cases, but I found that on the server with Consul Democracy 2.0.1, the fnm entries were not added to the deploy user
I think adding those entries to the deploy user |
I agree! No idea what's going on here 🤔. Maybe next week we can share a screen and try to find out. |
f15474a
to
d6eb6ce
Compare
Hi @javierm , I was wondering exactly which application dependencies need the Execjs runtime and why we need them. We have five dependencies that use Execjs for asset precompilation purposes (except eslintrb):
Quoting ExecJS docs:
I verified this behaviour by removing all my local NodeJS versions and checking that the runtime in my development environment changed from Before migrating to a specific version of NodeJS and allowing the use of npm packages in the Rails Asset Pipeline, we used the Execjs just for assets precompilation purposes. Now, we want to load npm packages in the Rails Assets Pipeline from a particular NodeJS version. Using other NodeJS versions could lead to unexpected results. I think now we should force the Execjs runtime environment so if developers do not have NodeJS installed, they get a very explicit error. We can do it with an initializer so any environment would fail if they do not find the runtime we want to use: config/initializers/execjs.rb
With this change the error is more precise:
Another thing we can do is add the required NodeJS version in the README file and mention the need to install the package.json dependencies to precompile assets. What do you think? |
@Senen I've updated the code:
Tried every scenario I could think of 🤞. |
I've updated the README to add the required Node.js version 👍. Not sure about the package.json dependencies, but I guess that one belongs to pull request #5159 🤔. |
Good question 🤔. |
We're choosing version 18 because if offers support for SSL 3, just like Ruby 3.1 does. Note we're symlinking a .nvmrc file as well, in order to make it compatible with NVM. While the .nvmrc and .node-version files use different formats, they both support the syntax we're using to define the version. The code to install Node.js in the Dockerfile is a simplification of the code in the Rails Dockerfile template [1]. [1] https://github.com/rails/rails/blob/04c97aec8a/railties/lib/rails/generators/rails/app/templates/Dockerfile.tt#L25
We use FNM instead of NVM. Reason: the setup seems easier with just `eval "$(fnm env)"`. So now, we try to install Node.js; if the command fails, it could be because FNM isn't installed (and we need to install it) or because the version of Node.js cannot be installed with the current version of FNM (and we need to update FNM). After installing/updating FNM, we try to install Node.js again. Note we're using `fnm env` in the middle of the `fnm_setup_command`. That way, the command will raise a `SSHKit::Command::Failed` exception if `fnm` isn't installed, and it will give us an indicator that we need to actually install it.
This code is based on what the rvm1-capistrano and capistrano-nvm gems do, but simplified a bit to take advantage of the `fnm exec` command. Since ExecJS will check for a JavaScript runtime every time the application is started, we need to include commands like `bundle` (used when running `bin/delayed_job`), `puma` and `rake`, so Node.js is found when running these commands. I'm not sure whether `pumactl` is also necessary, but I've added it for consistency. We're also including the default commands gems like capistrano-nvm use because we will add the `npm` command in the near future. Note that, in order to setup FNM, we need to enter the actual release folder (using `within release_path` isn't enough). So we have to run this task after the actual release is created; otherwise many commands would run in the previous release's folder.
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.
Hi @javierm,
As we talked about, if we finally decide to set the Execjs runtime to NodeJS, we'll do it in a separate PR.
@Senen We finally made it! 🎉 |
References
Objectives
Documentation
We need to update the technical documentation in order to add instructions about how to install a Node Version Manager and use it to install Node.js and to reflect the fact that, due to this change, Ubuntu 18.04 is no longer supported.
Release Notes