Skip to content
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

Notify the user that they're losing access to global modules #631

Merged
merged 8 commits into from
Feb 10, 2015

Conversation

ELLIOTTCABLE
Copy link
Contributor

This is a continuation of a discussion on #97.

When you install nvm, if you have any npm modules installed globally (with -g), they'll no longer be linked to the current install of Node and npm, nor is uninstalling them unintuitive.

Notifying the user of this fact seems like good practice.

@ELLIOTTCABLE
Copy link
Contributor Author

@ljharb I'm taking a second look at this today, and had a thought: switching from system to any other Node isn't that different from switching between arbitrary nodes.

I'm wondering if offering a simple, one-line summary of global binaries you're losing every time you switch Nodes isn't a bad idea? I mean, it could get noisy if you know what you're doing, so the way I'm thinking about doing this, is implementing it as a function in nvm, and then including that call to the function in the rc-file snippet, with a comment. That way, it's literally one line to remove from your own configuration, if you don't want the notifications.

Also, I'm new to your codebase; so let me know if you have any stylistic concerns. (I mostly tried to copy what I saw elsewhere in the code.)

So, don't merge this yet. Just continuing the conversation with some code.

@ELLIOTTCABLE
Copy link
Contributor Author

Also, I have no idea why the build is failing on Travis. The test I submitted passes locally on dash, bash, and zsh. Am I mis-understanding how your test-suite works? :x

@ljharb
Copy link
Member

ljharb commented Jan 24, 2015

Travis tests do occasionally fail. If one does, feel free to rerun it and it should pass.

I definitely agree that it makes sense that nvm use should report any global binaries that do not exist in both "previous" and "upcoming" nodes - ie if 0.10 has a, b, c, and 0.11 has b, c, d, and I'm using 0.10, nvm use 0.11 should print out Warning: v0.11.x does not have "a" installed globally..

I'm not sure what you mean about the rc-file snippet?

# Check whether the user has any globally-installed npm modules in their system
# Node, and warn them if so.
#
nvm_check_global_modules() {
Copy link
Member

Choose a reason for hiding this comment

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

Please ensure that any nvm_* functions defined are also added to the unset -f lists in this file, and/or in nvm.sh, whichever is appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

(specifically, please add nvm_check_global_modules to the list in nvm_reset

@ELLIOTTCABLE
Copy link
Contributor Author

I'm still working on this. I ran into a really bad bug. I suspect it is in npm or something, but it's crashing my Terminal every time I run my tests.

@ELLIOTTCABLE ELLIOTTCABLE force-pushed the notify-about-global-modules branch 2 times, most recently from 336ee4e to 058ef81 Compare February 3, 2015 02:43
@ELLIOTTCABLE
Copy link
Contributor Author

@ljharb forgive me for the slow response, I've not had a lot of time at the computer recently.

  1. Cleaned up what you'd commented on,
  2. Still, for the life of me, can't figure out what's causing these to pass locally and break on Travis. Can you run the install_script suite from my branch on your end, see if it's just me, or if it's just Travis?

@ljharb
Copy link
Member

ljharb commented Feb 7, 2015

Sure, just did. They do pass for me. I'll make suggestions for debugging/improvement on the test itself, which you can then try in travis.


npm install -g nop >/dev/null
message=$(nvm_check_global_modules)
[ ! -z "$message" ] || die "nvm_check_global_modules should have printed a notice when npm had global modules installed"
Copy link
Member

Choose a reason for hiding this comment

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

the opposite of ! -z is -n.

It may also be helpful here to run it first and do >/dev/null 2>&1 and assert that the output (stderr) is empty (-z).

Also, let's add $message into the die output, so we know what it did print out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Was intentionally using the same check, and negating it, amongst the various tests; I thought that was clearer to a non-initiate (I tend to believe the tests should be accessible to people that the code being tested isn't; as tests should be the entry-point for learning about the codebase). Let me know if this is unwelcome practice, I'll push a switch for -z and -n instead of -z and ! -z!
  2. I just pushed a bunch of debugging-related output, including showing the actual output of the check command on test failure. Covers this amongst other things~

@ELLIOTTCABLE
Copy link
Contributor Author

@ljharb sorry to take up so much of your time with such a trivial pull. Trying to familiarize myself with Travis as we go along.

Edit: Ah, nope, ignore that previous content. It's printing the info. Will keep banging away. <3

@ELLIOTTCABLE
Copy link
Contributor Author

Okay. I finally tracked down the bugs (I wasn't expecting versions of npm older than v2 in the wild, which was stupid of me.)

If you don't like the mini-debugging-framework (compatible with the popular node-debug library's approach) that I've included in install.sh, you can safely rebase commit c8efe3d out of the merge. Alternatively, if you do like it, I'm happy to take a moment and do the work of including it in nvm.sh as well.

Let me know if you've any other concerns!

@ljharb
Copy link
Member

ljharb commented Feb 8, 2015

Thanks! Yes, we need to support the npm that ships with every version of node :-)

I can see the value of the debugging stuff but I'd rather not keep it in the code. Do you mind rebasing it out? Otherwise I'll merge with the web UI and do a revert of it.

ljharb added a commit that referenced this pull request Feb 10, 2015
Notify the user that they're losing access to global modules
@ljharb ljharb merged commit 4768973 into nvm-sh:master Feb 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants