feat: implement lerna add <pkg>[@version] [--dev] by marionebl · Pull Request #1069 · lerna/lerna

marionebl · 2017-10-11T22:14:38Z

adresses Installing a new dependencies in projects with private dependencies that are not in NPM #998
implement AddCommand
add AddCommand unit tests
add lerna-add integration tests
add pkgMatchers for better manifest assertions

Description

Given

.
├── lerna.json
├── package.json
└── packages
    ├── package-1
    │   └── package.json # 1.0
    ├── package-2
    │   └── package.json # 2.0
    ├── package-3
    │   └── package.json # 1.0
    └── package-4
        └── package.json # 1.0

# adds @test/package-1@^1.0.0 to package-2,3,4
lerna add @test/package-1 

 # adds @test/package-1@~1 to package-2,3,4
lerna add @test/package-1@~1

# fails with ENOTFOUND
lerna add @test/package-5

# fails with ENOTSATISFIED
lerna add @test/package-1@2 

# noop
lerna add @test/package-1 --scope=@test/package-1 

# adds @test/package-1@^1.0.0 to package-2
lerna add @test/package-1 --scope=@test/package-2 

 # adds @test/package-1@^1.0.0 to package-3,4
lerna add @test/package-1 --ignore=@test/package-2

# @test/package-1 => 2,3,4
# @test/package-2 => 1,3,4  
lerna add @test/package-{1,2} --ignore=@test/package-2

Motivation and Context

adresses Installing a new dependencies in projects with private dependencies that are not in NPM #998
solution designed according to Installing a new dependencies in projects with private dependencies that are not in NPM #998 (comment)
focused on add as first step, remove and upgrade in separate PRs?

Before

To depend on local, unpublished dependency you have to
- add <dependency> to <depending>/package.json by hand
- execute lerna bootstrap --scope=<depending>

After

lerna add <dependency> --scope=<depending>

How Has This Been Tested?

Added unit tests
Added integration tests
Some smoke tests in private projects

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.

gausie · 2017-10-25T10:54:28Z

Is there anything stopping this being merged?

evocateur

Thanks for taking this on, @marionebl!

My apologies for the delay in review, I've been slammed at work.

evocateur · 2017-10-27T23:31:07Z

README.md

+
+When run, this command will:
+
+1. Add `package` to each applicable package. Applicable are packages that are not `package` and are in scope


We also need to support arbitrary 3rd-party packages, too. If --hoist is enabled, the external dependency should be installed in the root package.json first.

Guarding against installing a local dependency inside itself is good.

(it occurs to me that the --hoist behaviour is already dealt with by calling bootstrap at the end of execute, as long as we are also passing along this.options instead of the flags)

Addressed installing external dependencies via 4be0d91.

Does make a lot more sense from the user perspective, thanks for the review 👍 .

The change got bigger than I'd like due to the fact I switched to an async control flow.

This is needed due to the network requests we send to determine the correct remote package version ranges to persist.

This type of thing could be easier to build with async - await. Is there a reason is not used in the codebase so far?

evocateur · 2017-10-27T23:31:21Z

src/Command.js


  _logError(method, description, err) {
    log.error(method, description);
-


spaces are nice

Addressed via b34a031

evocateur · 2017-10-27T23:32:25Z

src/commands/AddCommand.js

+};
+
+export function handler(argv) {
+  new AddCommand([argv.script, ...argv.args], argv, argv._cwd)


What is argv.script here?

Adressed via e1d5fe5

evocateur · 2017-10-27T23:35:11Z

src/commands/AddCommand.js

+    return false;
+  }
+
+  constructor(input, flags, cwd) {


Command subclasses don't need constructors, it's all handled in the superclass.

Adressed via e6bfdff

evocateur · 2017-10-28T00:08:56Z

src/commands/AddCommand.js

+
+        const source = FileSystemUtilities.readFileSync(manifestPath);
+
+        const file = new JSONFile(manifestPath, source);


Is it really necessary to add JSONFile? You should be using write-pkg, an existing dependency, to write the package.json back to disk, as it already handles alphabetizing the updated (dev-)dependencies.

If you use read-pkg instead of reading the string, you can just merge applicable into the targeted dependencyType and write it back without testing, since the serialization is always alphabetized anyway.

const manifestJson = readPkg.sync(manifestPath, { normalize: false }); const manifestDeps = manifestJson[dependencyType] || {}; // overwrite updated dependencies manifestJson[dependencyType] = { ...manifestDeps, ...applicable, }; writePkg.sync(manifestPath, manifestJson);

I used json-file-plus to preserve package.json formatting. As this differs in behaviour from npm and other lerna commands and does not handle key alphabetization, I'll refactor as outlined.

evocateur · 2017-10-28T00:12:25Z

src/commands/AddCommand.js

+  }
+
+  packageSatisfied(name, versionRange) {
+    const pkg = this.packages.find(pkg => pkg.name === name);


Wondering why not const pkg = this.getPackage(name); here?

Addressed via a3a09a2

evocateur · 2017-10-28T00:19:16Z

src/commands/AddCommand.js

+  }
+
+  getPackage(name) {
+    return this.packages.find(pkg => pkg.name === name);


This and the body of packageExists() seems to wish this.packages was aMap()<String(name), Package>...

...or maybe Set()<Package>? Noodling for higher-level refactoring, no worries in this PR.

I had the impression that iterating over all (filtered) packages was more common in the code base, and <Package>[] makes that very easy.

evocateur · 2017-10-28T00:20:42Z

test/AddCommand.js

+
+expect.extend(pkgMatchers);
+
+describe("AddCommand", () => {


These are brilliant tests, thanks!

evocateur · 2017-10-28T00:21:52Z

test/helpers/pkgMatchers.js

@@ -0,0 +1,50 @@
+import semver from "semver";
+
+const matchDependency = dependecyType => {


should be dependencyType. I go cross-eyed typing "dependency" a lot 😭

Addressed via e4c9f0c

evocateur · 2017-10-28T00:23:46Z

test/integration/lerna-add.test.js

+    expect(pkgs['package-4']).toDependOn('@test/package-1');
+  });
+
+  test("add to scoped package", async () => {


Given the current functionality, I'm not sure we need any more integration tests beyond the first. (variations based on args and whatnot are already covered in unit tests)

Adressed via efa5cc3

* adresses lerna#998 * implement AddCommand * add AddCommand unit tests * add lerna-add integration tests * add pkgMatchers for better manifest assertions

evocateur · 2017-10-30T22:30:35Z

Sorry for the force push, I should really stop trying to use the dumb Github UI for resolving merge conflicts (it never, ever works)...

kachkaev · 2018-04-20T20:46:47Z

Thank you for for adding this feature @marionebl!

I noticed that placing --dev before package names fails the script and it took a while to investigate. Just curious: is this intentional?

## not in a mono-repo
yarn add something --dev ## works
yarn add --dev something ## works

## monorepo
lerna add something --dev ## works
lerna add --dev something ## ERROR: Missing list of packages to add to your project.

Should not --dev (as well as its alias -D) be used as flags so that they don't swallow an argument?

kachkaev · 2018-04-20T20:55:58Z

Created a new issue #1386

lock · 2018-12-27T05:28:01Z

This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs.

evocateur suggested changes Oct 28, 2017

View reviewed changes

evocateur added the Tag: Enhancement label Oct 28, 2017

marionebl force-pushed the feat-implement-lerna-add branch from 6ab20aa to 69672e1 Compare October 28, 2017 12:49

evocateur approved these changes Oct 30, 2017

View reviewed changes

marionebl added 15 commits October 30, 2017 15:28

feat: implement lerna add <pkg>[@Version] [--dev]

9dfd215

* adresses lerna#998 * implement AddCommand * add AddCommand unit tests * add lerna-add integration tests * add pkgMatchers for better manifest assertions

chore: update yarn.lock

0132c92

docs: describe lerna add usage

aa0cf90

style: readd blank line

5016a04

refactor: remove useless constructor

7a5f308

refactor: simplify BootstrapCommand creation

9507af7

test: remove duplicated --scope and --ignore tests

dfe1783

test: correct typo

867f244

fix: remove useless argv.script reference

3d93bde

fix: remove obsolete resolve-pkg devDependency

c22aba1

refactor: reuse AddComand.prototype.getPackage

7d9575a

refactor: remove readPkg, writePkg instead of json-file-plus

804516d

fix: move callback interface to outermost promise

8ffde59

fix: remove obsolete FileSystemUtilities import

b3242a8

feat: enable remote package installation via add

9254413

evocateur force-pushed the feat-implement-lerna-add branch from 2f04b10 to 9254413 Compare October 30, 2017 22:29

evocateur merged commit 8113903 into lerna:master Oct 30, 2017

marionebl mentioned this pull request Nov 1, 2017

[Feature] Adding cross-dependencies #53

Closed

ben-z mentioned this pull request Nov 1, 2017

lerna add removes all dependencies before adding new dependency #1099

Closed

ghost mentioned this pull request Nov 17, 2017

Update lerna in / from 2.2.0 to 2.5.1 src-zone/material#17

Closed

This was referenced Nov 18, 2017

[question] Using yarn commands with non-published lerna dependencies #501

Closed

Feature request: commands to add/remove dependencies from managed packages #833

Closed

This was referenced Feb 2, 2018

chore(deps): update dependency lerna to v2.9.0 TylorS/typed-unmaintained#47

Closed

chore(deps): update dependency lerna to v2.11.0 motorcycle/motorcycle.ts#256

Open

lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018


		When run, this command will:

		1. Add `package` to each applicable package. Applicable are packages that are not `package` and are in scope


		_logError(method, description, err) {
		log.error(method, description);


		const source = FileSystemUtilities.readFileSync(manifestPath);

		const file = new JSONFile(manifestPath, source);

		@@ -0,0 +1,50 @@
		import semver from "semver";

		const matchDependency = dependecyType => {


		expect.extend(pkgMatchers);

		describe("AddCommand", () => {

Conversation

marionebl commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Before

After

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

gausie commented Oct 25, 2017

Uh oh!

evocateur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marionebl Oct 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evocateur commented Oct 30, 2017

Uh oh!

kachkaev commented Apr 20, 2018

Uh oh!

kachkaev commented Apr 20, 2018

Uh oh!

lock bot commented Dec 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marionebl commented Oct 11, 2017 •

edited

Loading

marionebl Oct 28, 2017 •

edited

Loading