WIP: feat(remove): initial commit for lerna remove #1887

tanhauhau · 2019-01-23T09:44:54Z

I copied some of the code from @lerna/add, supporting glob and scope for filtering folders

Description

Current state:

Filter packages to be changed, based on filterOptions and path matching with glob
Determine whether the dependencies is external or internal
- Should determine this in package level, instead of project level, since if version doesn't match, it will not symlink
Update package.json
if the dependency is external:
- Remove symlink
else:
- call npm uninstall or yarn remove
- taking care of hoist logic
test
documentation

Motivation and Context

#1886

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

tanhauhau · 2019-01-23T09:48:33Z

There's a lot yet to be done, would like to listen to your input on this @evocateur

evocateur

We definitely don't need the npm-uninstall subpackage. Definitely needs some tests, and perhaps thinking about abstracting common patterns from lerna add as well...

evocateur · 2019-01-29T18:11:41Z

commands/remove/package.json

+  "dependencies": {
+    "@lerna/command": "file:../../core/command",
+    "@lerna/filter-options": "file:../../core/filter-options",
+    "@lerna/npm-uninstall": "file:../../utils/npm-uninstall",


@lerna/npm-uninstall looks like a copypaste of @lerna/npm-install. Why do we need it? We're already removing the entry in the package.json, etc. Running lerna bootstrap (like the add command does) should be sufficient to clean up the other stuff.

not really, the bootstrap command will not update the lockfile, at least that's not happening in yarn, due to https://github.com/lerna/lerna/blob/master/commands/bootstrap/index.js#L171

i guess that's the main motivation of the MR, because removing dep in package.json and running lerna bootstrap will not remove the dep from node_modules nor updating the lock file.

Err, that branch is only reached when you are attempting a filtered bootstrap. (scope, ignore, or since)

The chained bootstrap should have none of these options, thus the raw bootstrap should actually deal with this?

oh, my bad.
still, bootstrap will have early return if all the stated dependencies have been satisfied.
for our case (lerna remove), all stated dependencies is satisfied, but we have extraneous dependencies in node_modules, should we

reuse bootstrap, but add a new flag, (something like Running lerna bootstrap with node_modules doesn't update lockfile #1883 (comment)) to bypass the early return of lerna bootstrap, thus able to trigger npm client to remove extraneous pacakges and update lockfile or

use npm-uninstall that focuses on removing packages, by triggering npm client's remove/uninstall command rather than install/ci in lerna bootstrap

Oh, right, I forgot Yarn doesn't want you to remove stuff when you modify package.json and run yarn install...

So yeah, I guess we do need npm-uninstall. I'd prefer it was less copypasta, but that's more work that isn't blocking.

We also, however, need to account for the --hoist case (the removed dependency doesn't exist at the leaf, but in the root) and relative file: specifiers, which need only re-run npm install in the root. lerna bootstrap already accounts for the file: specifier case, but I really don't want to burden that poor command with any more complexity...

For the hoist case, how should we figure out whether the dependency to be removed is hoisted or installed in leaf node?

evocateur · 2019-01-29T18:13:35Z

commands/remove/index.js

+    //       args: [],
+    //       cwd: this.project.rootPath,
+    //       // silence initial cli version logging, etc
+    //       composed: "add",


We should definitely still be doing this, instead of calling npm uninstall. Ensuring that ci: false is also passed makes sure we run npm install which will synchronize the lockfile and node_modules appropriately?

evocateur · 2019-01-29T18:14:21Z

commands/remove/index.js

+    });
+  }
+
+  removeSymlink() {


We're not removing the symlink, but the directory. In any case, I don't think we need this. Just bootstrap after serializing the modified dependencies.

commands/remove/yarn.lock

tanhauhau · 2019-02-22T09:34:07Z

@evocateur

i've did some refactoring, extracting out @lerna/use-temporary-manifest, so that can be use when npm install and npm uninstall.
- I might have break some test, I'll fix those later
I've added some basic test for lerna remove
- going to add accout to --hoist case, and file: case

But I would like to listen to your comments on the general directions of the change, and there will be definitely a lot of parts of code in lerna remove and lerna bootstrap & npm-uninstall, I am going to extract them out into packages, I am not sure the packages structure I have in mind align with your plans.

tanhauhau · 2019-02-28T13:26:56Z

@evocateur what do you think?

lucaasrojas · 2019-03-20T17:06:28Z

This PR's idea is be able to remove packages from the monorepo?

tanhauhau · 2019-03-21T00:43:20Z

@lucaasrojas yes

montogeek · 2020-05-25T14:59:37Z

@evocateur What do you think?

evocateur · 2020-06-07T21:52:24Z

I don't really see this going forward anymore, sorry. I appreciate all the effort spent on this PR, but modern monorepos should be using package managers with proper capabilities to deal with this sort of thing (pnpm recursive, npm v7, yarn i guess).

Given the minimal frequency of use for this sort of command, the maintenance burden far outweighs the benefit of inclusion in the core. Perfect opportunity for a third-party utility.

davidmurdoch · 2020-11-07T19:00:59Z

Given the minimal frequency of use for this sort of command, the maintenance burden far outweighs the benefit of inclusion in the core. Perfect opportunity for a third-party utility.

FWIW, I would have used this command every day this week.

tanhauhau changed the title ~~WIP: initial commit for lerna remove~~ WIP: feat(remove): initial commit for lerna remove Jan 23, 2019

evocateur suggested changes Jan 29, 2019

View reviewed changes

tanhauhau and others added 3 commits February 17, 2019 10:35

initial commit for lerna remove

d96f9d2

remove accidentally commited yarn lock

9db4d56

remove libnpm

04be5c5

tanhauhau force-pushed the tanhauhau/lerna-remove branch from 0e83ddf to 04be5c5 Compare February 17, 2019 02:44

tanhauhau added 2 commits February 22, 2019 15:38

add use-temporary-manifest

764dbaf

! added test case

d4ba84b

evocateur added the WIP label Mar 31, 2019

evocateur closed this Jun 7, 2020

protyposis mentioned this pull request Oct 26, 2020

[BUG] Workspaces are not documented, don't appear to work npm/cli#2036

Closed

hinell mentioned this pull request Nov 6, 2020

Feature: lerna remove #1886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: feat(remove): initial commit for lerna remove #1887

WIP: feat(remove): initial commit for lerna remove #1887

tanhauhau commented Jan 23, 2019 •

edited

tanhauhau commented Jan 23, 2019

evocateur left a comment

evocateur Jan 29, 2019

tanhauhau Jan 30, 2019

tanhauhau Jan 30, 2019

evocateur Jan 31, 2019

tanhauhau Feb 1, 2019

evocateur Feb 1, 2019

tanhauhau Feb 1, 2019

evocateur Jan 29, 2019

evocateur Jan 29, 2019

tanhauhau commented Feb 22, 2019

tanhauhau commented Feb 28, 2019

lucaasrojas commented Mar 20, 2019

tanhauhau commented Mar 21, 2019 •

edited

montogeek commented May 25, 2020

evocateur commented Jun 7, 2020

davidmurdoch commented Nov 7, 2020

WIP: feat(remove): initial commit for lerna remove #1887

WIP: feat(remove): initial commit for lerna remove #1887

Conversation

tanhauhau commented Jan 23, 2019 • edited

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

tanhauhau commented Jan 23, 2019

evocateur left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanhauhau commented Feb 22, 2019

tanhauhau commented Feb 28, 2019

lucaasrojas commented Mar 20, 2019

tanhauhau commented Mar 21, 2019 • edited

montogeek commented May 25, 2020

evocateur commented Jun 7, 2020

davidmurdoch commented Nov 7, 2020

tanhauhau commented Jan 23, 2019 •

edited

tanhauhau commented Mar 21, 2019 •

edited