remove npm dependency for prefix check #1597

wzrdtales · 2017-08-26T20:22:21Z

For description see reference.

Refers to #1596
Fixes #1596

ljharb

Please also fix the indentation.

ljharb · 2017-08-26T20:23:40Z

nvm.sh

+
+  declare -a npmrcs=("." "$HOME")
+
+  for npmrc in "${npmrcs[@]}"; do


declare is also bash-specific; nvm requires POSIX.

ljharb · 2017-08-26T20:49:09Z

nvm.sh

-      else
-        nvm_err "Run \`$NVM_COMMAND\` to unset it."
+
+  declare -a npmrcs=("." "$HOME")


this is bash-specific syntax; nvm requires POSIX.

wzrdtales · 2017-08-26T21:24:35Z

yeh, changed it just submitted the local version I have running though :)

ljharb · 2017-08-26T21:36:29Z

nvm.sh

+    fi
+  done
+
+  if [[ -n $NVM_NPM_PREFIX ]]; then


double brackets are bash-only syntax; it's not in POSIX.

got it looking at it

ljharb · 2017-08-26T21:37:37Z

nvm.sh

+  local npmrcs=". $HOME"
+
+  for npmrc in $npmrcs; do
+    if [ -f "${npmrc}/.npmrc" ]; then


what I do like about this PR in particular is adding this npmrc check.

What would you think about adding this check prior to the npm config get prefix check? That would enable users to get a faster warning when things were broken, pointing them at the exact file that needed to be modified - and it'd still go through the npm config check if this grep passed.

That would also serve as a more iterative approach towards removing or bypassing the config check altogether (which I haven't agreed to yet).

Well yes, but in that case if I understand your intention right npm config get prefix would be called for every start again. Then we're were we started. But as additional check yes, that could tell the user which config is the nasty one.

Yes; and it'd be a further thing that wouldn't have to be removed or bypassed even if the npm config call was.

In that case this information is alrready there as we can just set a variable to the current npmrc value before the break statement and use that below in the error message.

So added the suggested error here https://github.com/creationix/nvm/pull/1597/files#diff-e1e8e036966ad5aaf7709712cd5d8ee3R2115

wzrdtales · 2017-08-28T20:27:57Z

So as there has been a 2 day silence since the last time, how may we continue on this matter?

ljharb · 2017-08-28T20:35:33Z

My comment above is basically saying that I'd like to add an additional early npmrc check; but that I'm not comfortable removing the npm config get prefix call at this time. Are you willing to reduce this PR to just that additive change?

wzrdtales · 2017-08-28T21:01:16Z

@ljharb That eliminates the whole sense of the PR and I don't think it is quite healthy to stop the discussion here. So what would be needed to actually move forward and resolve this discussion to an actual solution that is benefitial for all of us? So for you as maintainer and the users not suffering from slow startups anymore.

ljharb · 2017-08-28T21:46:30Z

As I've said many times; the slowdown is absolutely worth it to ensure the prefix check continues.

I'm very enthusiastic about ways to improve the speed, however - and my belief is that adding your npmrc check, and then also adding an env var check, while leaving the prefix check intact for the time being will be a very useful test to see if there are any cases the prefix check is still required for.

However, I'm not going to remove it lightly - it's been there, with the slowdown, for 2 years now. If you can't use nvm with that check, then for the time being, you can't use nvm - but I hope we can come up with a healthier long term plan than that.

wzrdtales · 2017-08-28T22:02:13Z

So you made your position clear, so we wont come at any time to any decision in this scenario.

but I hope we can come up with a healthier long term plan than that.

Your suggestion, as you said, it have been 2 years now. So given your attitude towards this, it seems like the only solution would be if npm would either rewrite a whole lot of their library to actually act fast again. Or at least move the configuration part into a separate library and do it just for this part. So if you have anything concrete on your mind, I will be happy to hear your opinion how a long term plan could look like.

wzrdtales · 2017-08-28T22:14:00Z

Btw. I have been looking at the whole nvm script a bit more deeply to look for any other opportunities, maybe we can at least talk about adding a fast path when respawning a shell.

Just quickly pushed a first version, which is not really complete (as it would currently not respect the default alias at all and just gets the last used version from the use branch, that would need to be added though) and just a first that works, here is the commit wzrdtales@ca95a3f. I really wondered why the use logic is being used to always find the version stored under the alias. Is there any reason that nvm does not just stores it default config state somewhere and reuses it? I can also open another issue for this later.

ljharb · 2017-08-28T22:31:40Z

The default state is PWD-dependent, and since the default alias is stored on disk as well, it could change at any time, out-of-band from nvm. It always has to be read freshly from disk to ensure it's accurate.

wzrdtales · 2017-08-28T22:33:47Z

@ljharb How is it PWD dependent? Talking about .nvmrc? A check against the default alias if it deviates shouldn't be that hard to add though.

ljharb · 2017-08-28T22:36:14Z

Yes, .nvmrc.

wzrdtales · 2017-08-28T22:44:50Z

Got it, Also not a problem in that case, just checking against nvm_rc_version before using the stored state.

So anything else you could think of, that would possibly cause issues when a fast path like this is added, which falls back to the current methodoligy as soon as the alias/default deviates or .nvmrc was found?

ljharb · 2017-08-29T00:46:56Z

I don't think so - I'm all for adding fast paths! As long as they're not sacrificing correctness by solely bypassing slow paths :-)

ljharb · 2021-09-29T05:36:24Z

This was obviated by #2317.

remove npm dependency for prefix check

2e6e512

ljharb requested changes Aug 26, 2017

View reviewed changes

nvm.sh Outdated

declare -a npmrcs=("." "$HOME")

for npmrc in "${npmrcs[@]}"; do

Copy link

Member

ljharb Aug 26, 2017

Choose a reason for hiding this comment

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

declare is also bash-specific; nvm requires POSIX.

look in other pathes when the current fails to return a prefix

c7cac32

ljharb reviewed Aug 26, 2017

View reviewed changes

wzrdtales added 2 commits August 26, 2017 23:07

make the path array POSIX compliant

c9aaeff

remove tabs that trapped in

dca7716

ljharb requested changes Aug 26, 2017

View reviewed changes

wzrdtales added 3 commits August 27, 2017 00:00

remove double brackets

6903c10

fixed shellcheck errors

127eb41

add descriptive error message an replace test stub

4cdbdae

wzrdtales force-pushed the removeNpmForPerformance branch 2 times, most recently from ea34a86 to d0dca5f Compare August 26, 2017 23:56

fix test

be18696

wzrdtales force-pushed the removeNpmForPerformance branch from d0dca5f to be18696 Compare August 27, 2017 00:41

This was referenced Mar 12, 2018

WHY? wzrdtales/nvm-ng#2

Open

NVM getting very slow on startup in Bash #1277

Closed

ljharb force-pushed the master branch from 00ecf52 to 8e1e5df Compare April 29, 2020 22:37

ljharb force-pushed the master branch from a09e064 to 49e806c Compare February 1, 2021 22:36

ljharb force-pushed the master branch from d3710f8 to 07253ec Compare April 16, 2021 05:45

ljharb closed this Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove npm dependency for prefix check #1597

remove npm dependency for prefix check #1597

wzrdtales commented Aug 26, 2017

ljharb left a comment

ljharb Aug 26, 2017

ljharb Aug 26, 2017

wzrdtales commented Aug 26, 2017

ljharb Aug 26, 2017

wzrdtales Aug 26, 2017

ljharb Aug 26, 2017

wzrdtales Aug 26, 2017

ljharb Aug 26, 2017

wzrdtales Aug 26, 2017

wzrdtales Aug 26, 2017

wzrdtales commented Aug 28, 2017 •

edited

Loading

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017 •

edited

Loading

ljharb commented Aug 29, 2017

ljharb commented Sep 29, 2021


		declare -a npmrcs=("." "$HOME")

		for npmrc in "${npmrcs[@]}"; do

remove npm dependency for prefix check #1597

remove npm dependency for prefix check #1597

Conversation

wzrdtales commented Aug 26, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzrdtales commented Aug 26, 2017

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

wzrdtales commented Aug 28, 2017 • edited Loading

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017

ljharb commented Aug 28, 2017

wzrdtales commented Aug 28, 2017 • edited Loading

ljharb commented Aug 29, 2017

ljharb commented Sep 29, 2021

wzrdtales commented Aug 28, 2017 •

edited

Loading

wzrdtales commented Aug 28, 2017 •

edited

Loading