Fix npm install

[kwikwag]

There seems to have been a hiccup when refactoring the temp dir for NPM. See how the tempDir variable was modified -- the line to unzip then proceeds to unzip from the wrong location. This should solve #1209 -- untested. Need to make sure that the rest of the flow works...

[taozai123]

hope review asap

[BerkieBb]

Can confirm this fixes the issue of npm not being downloaded because it doesn't recognize the temporary folder. Built the source myself with this code to confirm. Thank you kiwkwag.

[dev2-rush]

like

[BinToss]

Looks good! I've mentally followed the code paths up to Line 799. I also compared a3ce311 to 7de073e for good measure. See https://diffy.org/diff/a336cbca25d7f.

---

For reviewers' convenience, the filesystem structure of the root directory:

> tree /F nvm-install-1862240460
Folder PATH listing for volume Blade 17
Volume serial number is B0CE-64DF
C:\USERS\NOAH\APPDATA\LOCAL\TEMP\NVM-INSTALL-1862240460
├───temp
│       npm-v6.14.11.zip
│
└───v14.16.0
    │   node64.exe
    │
    └───node_modules

Points of interest in which filesystem paths matter:

• L671: root is assigned os.MkdirTemp("", "nvm-install-*").
  • Example: %TEMP%\nvm-install-1862240460\.
• L741: web.GetNpm(root, getNpmVersion(version)) downloads the NPM ZIP to filepath.Join(root, "temp", "npm-v"+npmv+".zip")
  • Example: %TEMP%\nvm-install-1862240460\temp\npm-v6.14.11.zip.
• L746: tempDir is assigned os.MkdirTemp("", "nvm-npm-*").
  • Example: %TEMP%\nvm-npm-0123456789\
• L759: tempNpmBin is assigned filepath.Join(tempDir, "nvm-npm", "cli-"+npmv, "bin"), but "cli-" will be replaced with "npm-" for NPM <6.2.0 (at L763).
  • Examples:
    • if NPM >= 6.2.0: %TEMP%\nvm-npm-0123456789\nvm-npm\cli-6.2.0\bin\
    • if NPM < 6.2.0: %TEMP%\nvm-npm-0123456789\nvm-npm\npm-6.1.0\bin\
• L772-L779: the npm, npm.cmd, npx, and npx.cmd files are moved from tmpNpmBin to ${root}\v${version} wherein version is the Node.js version`
• L781-L785, npmSourcePath is assigned filepath.Join(tempDir, "nvm-npm", "npm-"+npmv), wherein "npm-" may be replaced with "cli-".
• L787: npmSourcePath is moved (renamed) to filepath.Join(root, "v"+version, "node_modules", "npm")
• L800: filepath.Join(root, "v"+version) (Node.js + NPM) is moved to env.root and the installation is complete.

[ShowMeBillyJo]

Just ran into this bug and it killed my vibe for nearly a whole day. Thanks for submitting the patch!

[kwikwag]

Hey @BinToss, thanks for the review. Can you please instruct me how to proceed to get this merged?

[BinToss]

Hey @BinToss, thanks for the review. Can you please instruct me how to proceed to get this merged?

Now we wait for a maintainer to approve a review. Third parties can submit reviews, but cannot submit Approval reviews.

[libugo]

🆘

[Manish-Giri]

Any update on this? The PR has been open since 5 months.

[coreybutler]

This PR won't be used because the fix is more involved than a one-liner. I've already completed the work, but the release is held up by the arm64 build. GitHub recently added support for this in GitHub Actions, but I've exceeded the maximum GitHub Actions minutes with other projects over the last two months. I anticipate getting this build process complete this month, but my primary focus has been on Author/Runtime (the successor to this project).

Please remember, I am the only person doing any reviews/support on this. My time is finite and my primary focus is on what's next (because I'm as tired of the slow update cycles as everyone else is). I appreciate everyone's understanding.