-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
feat(env): allow installing and removing multiple NodeJS versions at once #7155
Conversation
💖 Thanks for opening this pull request! 💖 |
const message = await downloadNodeVersion(opts, version) | ||
if (message instanceof Error) { | ||
globalWarn(message.message) | ||
errors.push(message) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will make more sense to throw an error in downloadNodeVersion and catch it here. Also less code in envUse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not partial to exceptions as control flow...
Really bypasses type safety.
But that is already what a lot of this code is doing...
😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then return null instead of an error.
Congrats on merging your first pull request! 🎉🎉🎉 |
Thanks for taking the time to wrap it up. I have other changes I want to make and this first PR really helped get a better feel for the style you'd want. |
This PR:
env rm
to accept multiple versions, removing all of them at onceenv add
to allow installing multiple versions at once without setting them as default (like in the case ofenv use
)Some refactoring to dedupe code. I could go further with that, but I was trying to preserve as much of the existing structure and style as possible along the way. Not sure how opinionated the team is on that structure, or if a more aggressive sweep could make sense