feat: add --reinstall-packages-from= argument for fnm install#510
feat: add --reinstall-packages-from= argument for fnm install#510libook wants to merge 1 commit into
--reinstall-packages-from= argument for fnm install#510Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/schniz/fnm/FVX9uLJzzvvQ3eZ1Cq26FGeFTony |
There was a problem hiding this comment.
Hi @libook! THANK YOU for the contribution!
As I already mentioned in the thread, I am pretty torn about this feature: it really feels out of fnm's scope. You can even see that it is very third party in the code you just wrote here, and can be easily migrated to be a script like:
#!/bin/sh
fnm install $1
PACKAGES=$(fnm exec --using=$2 npm list -g --depth=0 2>/dev/null | ...)
fnm exec --using=$1 npm install -g $PACKAGESSo I'm not entirely sold in favor of maintaining this. What do you think? What are the pros (and if you can, the cons) of adding this?
Also, since fnm supports Windows, we should support this feature for Windows users too. Taking all of this code to another Rust module that is properly tested using unit/e2e tests will allow CI to run the code on Linux/Mac and Windows machines.
thanks again for your contribution and I'm really sorry if I'm a serious downer.
| let output = std::process::Command::new("sh") | ||
| .arg("-c") | ||
| .arg(format!( | ||
| concat!( | ||
| "fnm use \"{}\" >/dev/null ", | ||
| "&& npm list -g --depth=0 2>/dev/null ", | ||
| "| command sed 1,1d ", | ||
| "| command sed ", | ||
| "-e '/ -> / d' ", | ||
| "-e '/\\(empty\\)/ d' ", | ||
| "-e 's/^.* \\(.*@[^ ]*\\).*/\\1/' ", | ||
| "-e '/^npm@[^ ]*.*$/ d'", | ||
| ), | ||
| user_version, | ||
| )) | ||
| .output().expect("Failed to get package list."); |
There was a problem hiding this comment.
This will not work on Windows-based shells (wincmd).
Maybe we can try adding a new command, fnm reinstall-packages-from, and we can add unit/e2e tests for it.
| #[snafu(display("Execute command failed:\n{}", stderr))] | ||
| CommandExecutionFailed { | ||
| stderr: String, | ||
| }, |
There was a problem hiding this comment.
Please be more specific. InstallingPackagesFromOtherVersionFailed
|
@Schniz You are right. I am an nvm user for many years. I use MacOs, Linux and WSL as developing environment. And have forgotten that nvm doesn't support windows. I am learning Rust. I found fnm in some Rust news. It is a real "fast node manager" as the name said. That is why I did this.
I agree with you. This feature requires both npm and node version manager. Npm can't do this by itself. Extra script is the only way, if a node version manager doesn't do this either. Maybe there will be a better way for cross platform supporting and even for multiple node package managers too(e.g. yarn and pnpm) in the future. I am happy with learning and hacking Rust in this work. And I have learned a lot. I am ok if you think that it is not the time for this feature. |
|
@libook the next version of nvm does in fact support windows, the current one has supported WSL for years. |
|
@ljharb Wow, a familiar avatar. I always see you in ECMA works. Yes, I have been using nvm for years. Nvm has powerful functions. I have a zsh tool box which requires nvm as an optional dependency for scripts. Thank you guys for maintaining cool things. |
I am a noob in Rust.
My code may not be good enough. Please feel free to left your comments.