Lazy load nvm #1186

rtfpessoa · 2016-08-26T15:26:18Z

nvm takes on average half of a second to load, which is more than whole prezto takes to load.

This can be noticed when you open a new shell.

To avoid this, we are creating placeholder function for nvm, node, and all the node packages previously installed in the system to only load nvm when it is needed.

This code is based on the scripts:

nvm takes on average half of a second to load, which is more than whole prezto takes to load. This can be noticed when you open a new shell. To avoid this, we are creating placeholder function for nvm, node, and all the node packages previously installed in the system to only load nvm when it is needed. This code is based on the scripts: * https://www.reddit.com/r/node/comments/4tg5jg/lazy_load_nvm_for_faster_shell_start/d5ib9fs * http://broken-by.me/lazy-load-nvm/ * nvm-sh/nvm#781 (comment)

leoj3n · 2016-08-27T08:45:33Z

modules/node/init.zsh

+
+  for cmd in "${NODE_GLOBALS[@]}"; do
+    # Skip defining the function if the binary is already in the PATH
+    if ! which ${cmd} &>/dev/null; then


Nice, I was wondering how you were going to handle package binaries like https://www.npmjs.com/package/which

I am also trying #1178. Nodenv seems much faster on init and uses a different method for managing different node versions.

Since I already use rbenv which is much better than rvm and nodenv is a fork. I decided to give it a try.

Seems good.

I've had great success with nodenv and started using it because I was also familiar with rbenv. I too tried nvm and also found it noticeably slowed down my shell.

This could probably be improved by using $+commands[$cmd].

Let me know if you want any change I can update

Depending on what you're trying to do, is-callable from the helper module or $+commands would be preferable.

joaosa · 2016-09-25T20:59:21Z

@sorin-ionescu Any chance this could get merged? :)

before:

/usr/bin/time zsh -i -c exit
2.75 real         1.20 user         1.00 sys

after:

/usr/bin/time zsh -i -c exit
0.63 real         0.25 user         0.38 sys

ascandella · 2016-09-25T21:01:55Z

I also run a custom fork solely so I can include this patch.

facastagnini · 2016-12-29T05:44:51Z

This PR got merged in the community maintained fork of this repo.

zsh-users is an organization dedicated to host Zsh community projects and therefore became the rational home for a community maintained version of prezto.

Your contribution is greatly appreciated!

https://github.com/zsh-users/prezto

belak

There are a few tweaks I'd like to see, but the general concept looks good.

belak · 2017-05-02T23:39:54Z

modules/node/init.zsh

+  NVM_DIR=$1
+  # Skip adding binaries if there is no node version installed yet
+  if [ -d $NVM_DIR/versions/node ]; then
+    NODE_GLOBALS=(`find $NVM_DIR/versions/node -maxdepth 3 \( -type l -o -type f \) -wholename '*/bin/*' | xargs -n1 basename | sort | uniq`)


I'd be willing to bet that this doesn't work in either osx or linux. There should be a way to use extended glob syntax to do something like this but only use zsh internals.

What would be the part not working in OSX or Linux?

After looking a bit closer, it looks fine. I've just run into strange issues with find on linux and osx being incompatible.

belak · 2017-05-02T23:41:00Z

modules/node/init.zsh

+  for cmd in "${NODE_GLOBALS[@]}"; do
+    # Skip defining the function if the binary is already in the PATH
+    if ! which ${cmd} &>/dev/null; then
+      eval "${cmd}() { unset -f ${cmd} &>/dev/null; [ -z \${NVM_LOADED+x} ] && load_nvm; ${cmd} \$@; }"


Unsetting this specific command and using NVM_LOADED shouldn't be needed because we unset all the functions in load_nvm anyway, so we don't need to track it.

Additionally, because this is using eval, I'd feel a lot better if there was a way to know that the command names were sanitized.

Any suggestions?

ericbn · 2017-05-20T14:10:14Z

Replaced nvm by nodenv and my startup time when from 2s to 0.2s. Why bother about nvm?

belak · 2017-06-19T08:26:57Z

What's the status on this? It doesn't look like there's been an update by the author more than a month after changes were requested. Does anyone else want to take this up?

rtfpessoa · 2017-06-19T09:31:49Z

I completely missed this @belak.

It will probably take me some days to update this. If you want to do it I added permissions for the owners to edit this pr.

belak · 2017-06-19T09:34:09Z

No worries. Thanks for the response! I don't know if I'll get to it soon either, so no rush. Just trying to follow up on a few loose ends. :)

jeffwidman · 2017-10-04T18:32:00Z

I'm hesitant about this, and I'm hoping @creationix would be willing to take a look before we merge this.

I used to follow the nvm repo for a while, and there were often users complaining about speed, but every time they suggested a "fix" @creationix pointed out problems with their solution. For example, the OP cites as inspiration nvm-sh/nvm#781 (comment) but the very next comment is from the maintainer saying that that is a flawed approach.

That said, I don't work on any node projects right now, so haven't stayed up to date with nvm, so perhaps this implementation works fine, I just don't know and given the context I'd rather be cautious.

creationix · 2017-10-04T18:45:34Z

There has been quite a bit of feature and code growth since I handed nvm over to a new maintainer. If you look at the contribution graph my initial implementation is a fraction of the overall code.

If I were to write nvm again knowing what I now know, it would look a lot like nvs. https://github.com/jasongin/nvs

There is no need to have all the heavy logic in shell. The only thing that has to be shell script is changing the current PATH value. Everything else can be done in a sub-process on-demand.

creationix · 2017-10-04T18:49:04Z

I'm afraid I don't know enough about the current nvm implementation to give good feedback on your lazy load approach. Good luck!

jeffwidman · 2017-10-04T18:49:10Z

Thanks for weighing in @creationix! I actually meant to tag the new maintainer, as he is the one whose comments I've seen and didn't realize he wasn't you... so tag @ljharb 😄

ljharb · 2017-10-04T21:50:11Z

"code bloat" is pretty uncharitable; there's quite a lot of refactors, bug fixes, and POSIX robustness improvements in there :-)

Lazy loading nvm is imo not a good idea; it will either come with caveats, or break encapsulation by replicating the implementation.

The only reason it's slow is because npm config get prefix is slow, and people who have a "prefix" set with nvm run into very surprising and silent breakages, so it's worth the cost for the people who don't have a "prefix" set.

I'd be willing to bet that if you temporarily overwrote the $PATH with an npm that returned the expected value for npm config get prefix, then sourced nvm.sh greedily, and then removed your modification from $PATH, that it would be sufficiently fast that you wouldn't need to lazy load it at all.

creationix · 2017-10-05T01:25:11Z

@ljharb

"code bloat" is pretty uncharitable

Sorry, sometimes I forget the negative connotations with that phrase. You've done great work!

jeffwidman · 2017-10-05T21:11:20Z

Thanks for chiming in @ljharb, much appreciated

rtfpessoa mentioned this pull request Aug 26, 2016

Lazy load nvm #1185

Closed

leoj3n reviewed Aug 27, 2016
View reviewed changes

leoj3n mentioned this pull request Aug 30, 2016

Question caarlos0/dotfiles.zsh#256

Closed

facastagnini added a commit to prezto-inactive-community-fork/prezto that referenced this pull request Dec 29, 2016

Merge sorin-ionescu/pull/1186

ec6c512

belak requested changes May 2, 2017

View reviewed changes

rtfpessoa closed this Mar 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy load nvm #1186

Lazy load nvm #1186

rtfpessoa commented Aug 26, 2016

leoj3n Aug 27, 2016

rtfpessoa Aug 27, 2016

chauncey-garrett Sep 8, 2016 •

edited

belak Apr 13, 2017

rtfpessoa Apr 13, 2017

belak May 2, 2017

joaosa commented Sep 25, 2016

ascandella commented Sep 25, 2016

facastagnini commented Dec 29, 2016

belak left a comment

belak May 2, 2017

rtfpessoa Jul 15, 2017

belak Sep 19, 2017

belak May 2, 2017

belak May 2, 2017

rtfpessoa Jul 15, 2017

ericbn commented May 20, 2017

belak commented Jun 19, 2017

rtfpessoa commented Jun 19, 2017

belak commented Jun 19, 2017

jeffwidman commented Oct 4, 2017 •

edited

creationix commented Oct 4, 2017 •

edited

creationix commented Oct 4, 2017

jeffwidman commented Oct 4, 2017

ljharb commented Oct 4, 2017

creationix commented Oct 5, 2017

jeffwidman commented Oct 5, 2017

Lazy load nvm #1186

Lazy load nvm #1186

Conversation

rtfpessoa commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chauncey-garrett Sep 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaosa commented Sep 25, 2016

ascandella commented Sep 25, 2016

facastagnini commented Dec 29, 2016

belak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericbn commented May 20, 2017

belak commented Jun 19, 2017

rtfpessoa commented Jun 19, 2017

belak commented Jun 19, 2017

jeffwidman commented Oct 4, 2017 • edited

creationix commented Oct 4, 2017 • edited

creationix commented Oct 4, 2017

jeffwidman commented Oct 4, 2017

ljharb commented Oct 4, 2017

creationix commented Oct 5, 2017

jeffwidman commented Oct 5, 2017

chauncey-garrett Sep 8, 2016 •

edited

jeffwidman commented Oct 4, 2017 •

edited

creationix commented Oct 4, 2017 •

edited