-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add support for ~/nvm/default-packages file #1463
Conversation
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.
Thanks for the contribution! This is an interesting idea. I also like the file-based approach.
I'm concerned about adding the complexity of this feature to an already complex install process.
What about npm link
ed items, which nvm reinstall-packages
installs?
I do see in your example how a version range might be specified; what happens if an item is invalid? What happens if an item has spaces in it (which could cause npm
to treat it as multiple arguments)?
What about packages where the package is only compatible with a certain node version range, or, where the version number that is compatible depends on the node version? For example, I wouldn't want to install eslint
in a node version older than 4
.
This would also need unit tests to be able to be merged.
nvm.sh
Outdated
DEFAULT_PACKAGES="${DEFAULT_PACKAGES}${line[0]} " | ||
done < "${NVM_DIR}/default-packages" | ||
|
||
if [ "$DEFAULT_PACKAGES" != "" ]; then |
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.
this should probably use [ -z "${DEFAULT_PACKAGES}" ]
nvm.sh
Outdated
while IFS=" " read -r -a line; do | ||
|
||
# Skip empty lines. | ||
[ "${#line[@]}" -gt 0 ] || continue |
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.
would [ -z "${line[@]}" ]
work?
I agree we should not go about adding a bunch of unnecessary complexity with this. But from what I have added so far is not very complicated. For the lines with spaces in them, that should be invalid I assumed... We can easily provide an explanatory error for this but honestly this is something that is along the lines of garbage in, garbage out. The user should simply write a correct default-packages file. For the node version compatibility with packages specified. Adding this compatibility check, in my opinion, is adding too much complexity to be worth it unless you know of a dead simple way. But regardless, the most common use case for a version manager is upgrading to newer versions, not installing really old versions. For these cases we should provide a I will add the unit tests if you plan on merging this eventually. |
|
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.
Thanks! Sorry for the delay.
- The doctoc check is failing because when editing the readme, you'll need to run
npm run doctoc
to update the table of contents. - shellcheck is failing because you're using arrays, and nvm is implemented in POSIX, not bash/zsh - no arrays are available, sadly.
- The other failures seem like they might be legitimate; but probably easiest to clear up the others first and rerunning, before trying to debug.
README.markdown
Outdated
If you have a list of default packages you want installed every time you install a new version we support that too. You can add any valid package, version, repo, url for installing an npm package. | ||
|
||
```sh | ||
# ~/.nvm/default-packages |
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.
let's do $NVM_DIR/default-packages
here, just to try to avoid hard-coding the default
README.markdown
Outdated
@@ -216,6 +216,19 @@ nvm install 6 --reinstall-packages-from=5 | |||
nvm install v4.2 --reinstall-packages-from=iojs | |||
``` | |||
|
|||
### Default global packages from file while installing | |||
|
|||
If you have a list of default packages you want installed every time you install a new version we support that too. You can add any valid package, version, repo, url for installing an npm package. |
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.
"for installing an npm package - anything npm would accept as a package argument on the command line."?
nvm.sh
Outdated
PROVIDED_REINSTALL_PACKAGES_FROM="$(nvm_echo "$1" | command cut -c 27-)" | ||
REINSTALL_PACKAGES_FROM="$(nvm_version "$PROVIDED_REINSTALL_PACKAGES_FROM")" ||: | ||
;; | ||
--copy-packages-from=*) |
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.
i think adding these lines means we can remove the ones on lines ~2467?
|
||
# commented-package | ||
|
||
webpack/webpack |
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.
let's pick some smaller packages for the tests (than webpack or gulp) and still-relevant ones (unlike bower) - how about rimraf
, object-inspect
, things like that?
cleanup | ||
|
||
cat > ../../../default-packages << EOF | ||
not-a-package-name |
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.
this could be a valid package name - maybe not~a~package~name
or something?
not-a-package-name | ||
EOF | ||
|
||
OUTPUT="$(nvm exec 6.10.1 npm list -g | grep gulp)" |
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.
npm ls -g
can take a --depth=0
argument which should help the test be faster and more accurate
I have added the changes you requested. By the way, Is there a way I can run only the unit tests or only a specific unit test? |
.gitignore
Outdated
@@ -11,6 +11,7 @@ test/**/test_output | |||
|
|||
node_modules/ | |||
npm-debug.log | |||
.yarn-lock |
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.
yarn.lock
should be gitignored; but I'm not sure what .yarn-lock
is nor why it would occur in a project not using yarn?
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.
Just because I installed the devDependencies using yarn its faster, I thought about this already before adding it to this file. Its just a .gitignore file, let it ignore.
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.
Project-level gitignores should only contain project-related ignores; anything else should be in a global gitignore - otherwise you get every IDE's dotfiles polluting every repo.
If you wanted to ignore yarn.lock
I'd understand, but .yarn-lock
isn't a file that exists, as far as I'm aware.
nvm.sh
Outdated
@@ -2419,13 +2421,51 @@ nvm() { | |||
PROVIDED_REINSTALL_PACKAGES_FROM="$(nvm_echo "$1" | command cut -c 22-)" | |||
REINSTALL_PACKAGES_FROM="$(nvm_version "$PROVIDED_REINSTALL_PACKAGES_FROM")" ||: | |||
;; | |||
--skip-default-packages) | |||
SKIP_DEFAULT_PACKAGES=true ||: |
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.
||:
shouldn't be required here, since you're not doing anything that could possibly have a nonzero exit code.
nvm.sh
Outdated
*) | ||
ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1" | ||
;; | ||
esac | ||
shift | ||
done | ||
|
||
if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -f "${NVM_DIR}/default-packages" ]; then | ||
local DEFAULT_PACKAGES="" |
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.
please always declare local
vars in a separate line from initializing them; this line as-is won't work in ksh
which doesn't support local
.
nvm.sh
Outdated
{ | ||
nvm_echo "$DEFAULT_PACKAGES" | command xargs npm install -g --quiet | ||
} || { | ||
nvm_echo "Failed installing default packages. Please check if your default-packages file or a package in it has problems!" |
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.
this is error output; it should use nvm_err
instead of nvm_echo
|
||
die () { echo "$@" ; cleanup ; exit 1; } | ||
cleanup () { | ||
rm -rf "$(nvm_version_path v6.10.1)" ../../../default-packages |
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.
tests are often run inside a working nvm installation; if I have a pre-existing default-packages
file, this test will clobber it.
@westonganger typically i just |
nvm.sh
Outdated
[ -z "${line}" ] || continue | ||
|
||
# Skip comment lines that begin with `#`. | ||
[ "${line:0:1}" != "#" ] || continue |
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.
the shellcheck job failed; it says string indexing isn't supported in dash.
That makes testing much faster. Thanks! I have now updated this with changes you requested. |
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.
Thanks, this looks great!
There's some minor changes I can make on my own as I merge this.
.gitignore
Outdated
@@ -11,6 +11,7 @@ test/**/test_output | |||
|
|||
node_modules/ | |||
npm-debug.log | |||
.yarn-lock |
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.
Project-level gitignores should only contain project-related ignores; anything else should be in a global gitignore - otherwise you get every IDE's dotfiles polluting every repo.
If you wanted to ignore yarn.lock
I'd understand, but .yarn-lock
isn't a file that exists, as far as I'm aware.
README.markdown
Outdated
|
||
gulp | ||
webpack | ||
bower@1.7.1 |
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.
let's avoid using bower as an example here too (i can update this myself prior to merging tho, not a blocker)
nvm.sh
Outdated
*) | ||
ADDITIONAL_PARAMETERS="$ADDITIONAL_PARAMETERS $1" | ||
;; | ||
esac | ||
shift | ||
done | ||
|
||
if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -f "${NVM_DIR}/default-packages" ]; then | ||
local DEFAULT_PACKAGES | ||
|
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.
i'll remove this extra blank line also
|
||
# commented-package | ||
|
||
stevemao/left-pad |
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.
cute
.gitignore
Outdated
@@ -11,6 +11,7 @@ test/**/test_output | |||
|
|||
node_modules/ | |||
npm-debug.log | |||
.yarn-lock | |||
|
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.
we should probably ignore default-packages
as well, so contributors don't accidentally commit theirs.
Although, tests are still failing - so this will need to wait until they can be made to pass. |
I added the changes you have mentioned and continued working on the tests. I am kind of stuck. Getting the following error: |
nvm.sh
Outdated
if [ -z "$DEFAULT_PACKAGES" ]; then | ||
nvm_echo "Installing default global packages from ${NVM_DIR}/default-packages..." | ||
{ | ||
nvm_echo "$DEFAULT_PACKAGES" | command xargs npm install -g --quiet |
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.
ah - you're trying to npm install
before nvm use
is ran on line 2488.
I think you should extract this block of code to a separate function; and then call that function on line 2495.5.
I moved it to another function. Hopefully its what you meant. |
That looks great; there's other seemingly unrelated tests failing, presumably because they do Have you tested locally with |
I have now done all the local testing and came up with some glaring holes, oops... I patched that all up, enhanced and fixed the tests. It now works like a charm under all conditions. I think this one is ready to go! |
nvm.sh
Outdated
@@ -2441,9 +2471,12 @@ nvm() { | |||
FLAVOR="$(nvm_node_prefix)" | |||
fi | |||
|
|||
if nvm_is_version_installed "$VERSION"; then | |||
if nvm_is_version_installed "$VERSION" && nvm use "$VERSION"; then | |||
nvm_err "$VERSION is already installed." |
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.
it's very intentional that the error message prints only if nvm_is_version_installed
fails, and not if nvm use
fails.
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.
Good call was unsure about that one
nvm.sh
Outdated
nvm_echo "$1" | command xargs npm install -g --quiet && return 0 | ||
} || { | ||
nvm_err "Failed installing default packages. Please check if your default-packages file or a package in it has problems!" | ||
return 1 |
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.
maybe this should capture the exit code from npm install
?
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.
Ya this definately had weird style. I have updated it to be more standard.
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.
the "fast" tests are still failing; oddly, they're nvm_get_checksum_alg
and nvm_get_mirror
which are simple unit tests.
nvm.sh
Outdated
@@ -2511,6 +2546,9 @@ nvm() { | |||
else | |||
nvm_ensure_default_set "$provided_version" | |||
fi | |||
if [ -z "$SKIP_DEFAULT_PACKAGES" ] && [ -n "$DEFAULT_PACKAGES" ]; then | |||
nvm_install_default_packages "$DEFAULT_PACKAGES" | |||
fi | |||
if [ ! -z "$REINSTALL_PACKAGES_FROM" ] \ |
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.
not sure why i used ! -z
here, but this could become -n
too :-)
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.
lol. I looked at that but was like shit it aint broken, better not touch it
I removed Would you merge a different PR which removes all |
That means the |
Alright these are back. I dont know what to do about those tests nothing should have changed regarding those tests. |
Can this be merged now? |
@westonganger I'm not sure what to do about the tests either; the implication is that your changes have affected those code paths. I can't merge it unless tests are passing. |
So both of these tests run successfully on my local machine, why do they fail on the build? |
This looks really good! It would be great to manage the packages using nvm rather than editing the file, perhaps after this gets accepted. |
@westonganger so one of the failures is because your new function |
Thanks for the contribution, and for bearing with the review! |
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.
This is great. Thank you!
Thank you for adding this! |
Is there a better way to upgrade default packages besides |
@slapbox there isn't a separate command, no - we could certainly add one if there's a use case. file a new issue? |
This PR adds support for a file containing default packages to be installed with every new version install.
While the
reinstall-packages
option is great, this file based approach is nice because I can check this file into my personal dotfiles repo.Note: this idea was adapted from https://github.com/rbenv/rbenv-default-gems. I wanted a similar thing from my node version manager setup.