feat(env): add remove command to pnpm env #5263

mark-omarov · 2022-08-25T05:44:07Z

Summary

This PR enhances the pnpm env functionality with the uninstall command, allowing to uninstall Node.JS globally, that was previously installed with pnpm env use <version>.

Partially closes #5155

Listing installed Node.JS will be done in a separate PR, I'm trying to keep this one short.

Note to reviewers

I think there's room for refactoring, but for now, I just tried to follow what's already in place. If refactoring is an option, I could work on it furthermore.

welcome · 2022-08-25T05:44:15Z

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

packages/plugin-commands-env/src/env.ts

zkochan · 2022-08-25T09:14:55Z

If refactoring is an option, I could work on it furthermore.

you can do refactoring if you want.

mark-omarov · 2022-08-26T09:24:07Z

@zkochan I'll do the refactoring in a separate PR, maybe together with the pnpm env --global list command, just to keep this one simple.

I've noticed that one of the CI for Node.js 14.6 was failing, I went to check and figured there's Node.js API I was using that didn't exist back then, I've added a workaround for that specific API, hope that was worth it.

zkochan · 2022-08-26T12:21:31Z

I think remove with rm as a short alias would be a better name. But we can add the same aliases that the pnpm remove command has: remove, rm, uninstall, un

- Fix broken node path for the windows platform - Notify user when removing default Node.JS - Remove `await` on `fs.unlink` in `Promise.all` - Add support for Node.JS v14.13 and earlier

- Add `rm, un, uninstall` aliases to the `remove` command - Update CLI help message - Ignore only `ENOENT` errors

mark-omarov · 2022-08-27T04:12:37Z

@zkochan, I've made the necessary changes and adjusted the tests to pass on the Windows platform. I don't have one available for testing locally, will have to see if it's working on the CI.
Also, I've made some changes to the CLI help message, since now pnpm env is not only handles the installation, but allows removing as well, I think it's more appropriate to state that pnpm env is managing Node.JS versions, and not only installs them.
Please take a look, lmk if any adjustments are required. Thanks!

zkochan · 2022-08-27T12:06:44Z

packages/plugin-commands-env/src/env.ts

+            name: 'remove\nuninstall',
+            shortAlias: 'rm,\nun',


Just write remove and rm

Suggested change

name: 'remove\nuninstall',

shortAlias: 'rm,\nun',

name: 'remove',

shortAlias: 'rm',

zkochan · 2022-08-27T12:07:15Z

.changeset/long-trainers-share.md

+"@pnpm/plugin-commands-env": minor
+---
+
+Enhance `pnpm env` with the `uninstall` command.


Suggested change

Enhance `pnpm env` with the `uninstall` command.

Enhance `pnpm env` with the `remove` command.

zkochan · 2022-08-27T12:07:28Z

packages/plugin-commands-env/src/env.ts

+            name: 'use',
+          },
+          {
+            description: 'Uninstalls the specified version of Node.JS.',


Suggested change

description: 'Uninstalls the specified version of Node.JS.',

description: 'Removes the specified version of Node.JS.',

zkochan · 2022-08-27T12:08:24Z

packages/plugin-commands-env/src/env.ts

+      ]).catch(err => {
+        const { code = '' } = err
+
+        if (code.toLowerCase() !== 'enoent') {


It will be also upper case. No need to lower it

Suggested change

if (code.toLowerCase() !== 'enoent') {

if (code !== 'ENOENT') {

zkochan · 2022-08-27T12:09:28Z

packages/plugin-commands-env/src/env.ts

+      await Promise.all([
+        fs.unlink(nodePath),
+        fs.unlink(npmPath),
+        fs.unlink(npxPath),
+      ]).catch(err => {


just use a try/catch syntax

Suggested change

await Promise.all([

fs.unlink(nodePath),

fs.unlink(npmPath),

fs.unlink(npxPath),

]).catch(err => {

try {

await Promise.all([

fs.unlink(nodePath),

fs.unlink(npmPath),

fs.unlink(npxPath),

])

} catch (err) {

if (err.code !== 'ENOENT') throw err

zkochan · 2022-08-27T12:10:21Z

packages/plugin-commands-env/src/env.ts

+    /**
+     * Support for earlier Node.JS versions
+     * `fs.rmdir` is deprecated and `fs.rm` was added in v14.14.0
+     * @see https://nodejs.org/dist/latest-v16.x/docs/api/fs.html#fsrmpath-options-callback
+     */
+    if (Number(processMajorVersion) === 14 && Number(processMinorVersion) <= 13) {
+      await fs.rmdir(versionDir, { recursive: true })
+    } else {
+      await fs.rm(versionDir, { recursive: true })
+    }


Just use this rimraf lib:

pnpm/packages/core/package.json

Line 56 in d904b83

"@zkochan/rimraf": "^2.1.2",

- Use `remove` with `rm` alias only - Update CLI help message - Update tests to use `remove` command - Replace custom support for older Node.JS versions with `@zkochan/rimraf`

zkochan · 2022-08-28T20:05:28Z

packages/plugin-commands-env/test/env.test.ts

+
+    const opts = {
+      env: {
+        [PATH]: `${process.cwd()}${path.delimiter}${process.env[PATH] as string}`,


I don't understand how this test works. Even if you remove the node.js from home path, won't execa pick the system installed node.js?

I'm sorry, I didn't have the capacity to respond yesterday.

So, you're correct, it should have picked other (in my case nvm) installed versions. I was going to restrict path to only the current folder, but I see that you've done that already.

I think it's worth mentioning that execa, for some reason (I didn't get the chance to investigate further yet) tries to resolve the "node" in the temp folder first, I'm not sure why that's happening exactly.

Anyway, thanks for merging!

welcome · 2022-08-29T22:53:15Z

Congrats on merging your first pull request! 🎉🎉🎉

ryuujo1573 · 2023-02-22T06:30:13Z

my pnpm is died 😭 help
#6122

mark-omarov requested a review from zkochan as a code owner August 25, 2022 05:44

zkochan reviewed Aug 25, 2022

View reviewed changes

zkochan added the area: env label Aug 25, 2022

mark-omarov requested a review from zkochan August 26, 2022 09:20

mark-omarov added 5 commits August 27, 2022 12:19

feat(env): add uninstall command to pnpm env

ae714f4

chore(changeset): define minor bump

fc9895c

fix(env): fixes for pnpm env uninstall

9881273

- Fix broken node path for the windows platform - Notify user when removing default Node.JS - Remove `await` on `fs.unlink` in `Promise.all` - Add support for Node.JS v14.13 and earlier

fix(env): fix failing tests on Windows platform

f621290

fix(env): update aliases and error catching

31e4d25

- Add `rm, un, uninstall` aliases to the `remove` command - Update CLI help message - Ignore only `ENOENT` errors

mark-omarov force-pushed the enhance-env branch from 5ea3769 to 31e4d25 Compare August 27, 2022 04:05

zkochan reviewed Aug 27, 2022

View reviewed changes

mark-omarov added 2 commits August 28, 2022 12:24

refactor(env): cr updates

7056dca

- Use `remove` with `rm` alias only - Update CLI help message - Update tests to use `remove` command - Replace custom support for older Node.JS versions with `@zkochan/rimraf`

refactor(env): simplify catch block

a23eebf

mark-omarov changed the title ~~feat(env): add uninstall command to pnpm env~~ feat(env): add remove command to pnpm env Aug 28, 2022

mark-omarov requested a review from zkochan August 28, 2022 03:41

fix: env rm

fc94aa8

zkochan reviewed Aug 28, 2022

View reviewed changes

refactor: test

6d7ca26

zkochan approved these changes Aug 29, 2022

View reviewed changes

zkochan merged commit ba270a9 into pnpm:main Aug 29, 2022

mark-omarov deleted the enhance-env branch August 30, 2022 02:53

zkochan added this to the v7.10 milestone Sep 2, 2022

renovate bot mentioned this pull request Oct 3, 2022

chore(deps): update all non-major dependencies unjs/ipx#84

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(env): add remove command to pnpm env #5263

feat(env): add remove command to pnpm env #5263

mark-omarov commented Aug 25, 2022

welcome bot commented Aug 25, 2022

zkochan commented Aug 25, 2022

mark-omarov commented Aug 26, 2022 •

edited

zkochan commented Aug 26, 2022

mark-omarov commented Aug 27, 2022 •

edited

zkochan Aug 27, 2022

zkochan Aug 27, 2022

zkochan Aug 27, 2022

zkochan Aug 27, 2022

zkochan Aug 27, 2022

zkochan Aug 27, 2022

zkochan Aug 28, 2022

mark-omarov Aug 30, 2022

welcome bot commented Aug 29, 2022

ryuujo1573 commented Feb 22, 2023

	Enhance `pnpm env` with the `uninstall` command.
	Enhance `pnpm env` with the `remove` command.

	description: 'Uninstalls the specified version of Node.JS.',
	description: 'Removes the specified version of Node.JS.',

	if (code.toLowerCase() !== 'enoent') {
	if (code !== 'ENOENT') {

feat(env): add remove command to pnpm env #5263

feat(env): add remove command to pnpm env #5263

Conversation

mark-omarov commented Aug 25, 2022

Summary

Note to reviewers

welcome bot commented Aug 25, 2022

zkochan commented Aug 25, 2022

mark-omarov commented Aug 26, 2022 • edited

zkochan commented Aug 26, 2022

mark-omarov commented Aug 27, 2022 • 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

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

welcome bot commented Aug 29, 2022

ryuujo1573 commented Feb 22, 2023

mark-omarov commented Aug 26, 2022 •

edited

mark-omarov commented Aug 27, 2022 •

edited