Actually install the version of node we want via NVM/nodenv instead of just checking #434
Conversation
There was a problem hiding this comment.
nvm stuff LGTM; however, it seems like any script should assume the PATH is set up such that node and npm are the correct versions, and shouldn't get involved in package managers (of which you've chosen here the most popular/common, skipped the next-most-popular/common 2 or 3, and also chosen one of the least popular/common, which seems like an odd support matrix)
scripts/check-and-run-apollo-cli.sh
Outdated
| # Only major version should be specified here | ||
| REQUIRED_NODE_VERSION=8 | ||
| # Specify fully qualified version here | ||
| REQUIRED_NODE_VERSION=8.0.0 |
There was a problem hiding this comment.
I think sticking with the major is probably better; especially considering that 8.0.0 is EOL and only the latest 8 (8.15.0 at this writing) is considered LTS.
There was a problem hiding this comment.
Ah good context. Thanks @ljharb. Nodenv was requiring me to specify a fully qualified version. I’ll update this to be 8.15.0 since that is LTS.
|
@bachand: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
| elif [[ -x "$(command -v brew)" && -s "$(brew --prefix nvm)/nvm.sh" ]]; then | ||
| . "$(brew --prefix nvm)/nvm.sh" | ||
| use_correct_node_via_nvm | ||
| fi | ||
|
|
||
| # Set up the nodenv node version manager if present | ||
| if [[ -x "$HOME/.nodenv/bin/nodenv" ]]; then |
There was a problem hiding this comment.
Full transparency: I just installed a clean version of Nodenv and this file doesn't exist, so I am wondering if this check is incorrect. That being said, I've tested the commands here for installing Node 8.15.0 via Nodenv and they work. Determining if in fact there is an issue in this check should be a separate investigation / PR.
|
Thanks for the quick review @martijnwalraven. I have made the change suggested by @ljharb and signed the CLA. This is ready to be merged (from my perspective). I look forward to working more closely together on Apollo moving forward :) |
|
Thanks, looks good to me! Also looking forward to working together. |
|
Thanks for merging @martijnwalraven. Do you have a sense of when you'll be creating a new version? |
|
@bachand I just released version 0.9.5 with these changes. |
|
Thanks 🙏 |
👋
I am working on updating Airbnb to the latest version of Apollo. I was running into issues with this script.
The issue I faced was that sourcing
~/.nvm/nvm.shwas causing Node to be reset to the version specified in my.nvmrc. The version in there is 6.x.x, which is necessary for other tooling. I couldn't find a way to set Node to 8.x.x outside of this script since sourcing~/.nvm/nvm.shwould continually reset us back to 6.x.x.Since we have access to these version managers, let's just set up the version that we need so that the script can be run regardless of what Node versions you have installed. Making this change allowed me to remove the code that checks that the Node version is correct.
The one downside that I can see of this change is that if you had a version like 8.1.0 installed and active, the old script would have run fine since it was just checking the major version. As of this change, we will install and activate the exact version. I think there are pros/cons to each approach. The con of the new approach is that you may have to install a version of node that is very similar to one that you already have. The pro everyone is always running the same version of Node, which makes it easier to debug issues 🤷♂️
cc: @ljharb for NVM changes