Skip to content

Responses to PRs #48

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.

Sign up for GitHub

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

Open
brock wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Responses to PRs #48

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e9fde7
fixes #46
brock Dec 15, 2018
44e7862
fixes #38
brock Dec 15, 2018
53c920d
fixes #45
brock Dec 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions node-reinstall
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# credit: http://stackoverflow.com/a/11178106/2083544

## program version
VERSION="0.0.17"
VERSION="0.0.18"

## path prefix
PREFIX="${PREFIX:-/usr/local}"
Expand Down Expand Up @@ -105,7 +105,7 @@ confirm ()
sudo -v

# if node is installed, get the installed version
INSTALLED_NODE_VERSION=$(node --version 2> /dev/null)
INSTALLED_NODE_VERSION=$(node --version 2&1> /dev/null)
if [[ ! -z $INSTALLED_NODE_VERSION ]]; then
echo "Found a version of Node.js that is already installed."
echo "If you continue now, we will re-install Node.js version $INSTALLED_NODE_VERSION"
Expand Down Expand Up @@ -182,12 +182,18 @@ sudo rm -rf $PREFIX/bin/{node,npm}
sudo rm -rf $PREFIX/share/man/man1/node.1
sudo rm -rf $PREFIX/lib/dtrace/node.d

# remove global node_modules on arch
[ -d /usr/lib/node_modules ] && sudo rm -rf /usr/lib/node_modules

if (( $USE_NVM )); then
# go home and install NVM just because I feel safe there
cd $HOME
# get the latest stable version number of nvm from the repo's homepage
[ "$STABLE" == "" ] && STABLE=$(curl -s -k https://github.com/creationix/nvm/ | grep "curl https://raw.githubusercontent.com/creationix/nvm/" | grep -oE "v\d+\.\d+\.\d+")
[[ $STABLE =~ ^v[0-9]+.[0-9]+.[0-9]+$ ]] || STABLE="v0.25.1"
# get the latest stable version number of nvm from the repo's package.json
Copy link

@ljharb ljharb Dec 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very brittle. The previous approach is the one you should be using (getting a list of tags, sorting, and grabbing the last one).

Copy link
Owner Author

@brock brock Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to review this, @ljharb.

The previous version was just checking the README for the version, whereas this version gets the package.json and parses the JSON, which is arguably a bit safer than parsing a Markdown.

Having said that, I prefer the idea of fetching the tags on your repo and getting the latest stable version.

I'll look into this as an option. We are using your curl instructions now to just pull down the installer, but I think we could clone the repo and checkout the latest stable tag.

Thanks for the feedback.

Copy link

@ljharb ljharb Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly parsing the readme is brittle; using git tag output is robust.

# credit to https://unix.stackexchange.com/a/166360/107211 and
# credit to https://stackoverflow.com/a/6852427/2083544
[ "$STABLE" == "" ] && STABLE_WITHOUT_V=$(curl -v --silent https://raw.githubusercontent.com/creationix/nvm/master/package.json 2>&1 | grep -Eo '"version":.*?[^\\]",' | perl -pe 's/"version": //; s/^"//; s/",$//')
[ -n $STABLE_WITHOUT_V ] && [[ $STABLE_WITHOUT_V =~ ^[0-9]+.[0-9]+.[0-9]+$ ]] && STABLE="v${STABLE_WITHOUT_V}"
[[ $STABLE =~ ^v[0-9]+.[0-9]+.[0-9]+$ ]] || STABLE="v0.33.11"
curl -sL https://raw.githubusercontent.com/creationix/nvm/$STABLE/install.sh | bash
source $HOME/.nvm/nvm.sh
elif (( $USE_NAVE )); then
Expand Down