Pass extra arguments to npmClient during bootstrap #834

xaka · 2017-05-22T20:24:44Z

Description

Allow to pass npmClient arguments.

Motivation and Context

How Has This Been Tested?

# npm version
{ lerna: '2.0.0-rc.4',
  npm: '3.10.10',
  ares: '1.10.1-DEV',
  http_parser: '2.7.0',
  icu: '57.1',
  modules: '48',
  node: '6.9.4',
  openssl: '1.0.2j',
  uv: '1.9.1',
  v8: '5.1.281.89',
  zlib: '1.2.8' }

# uname -a
Darwin pstrashkin-mbp 15.6.0 Darwin Kernel Version 15.6.0: Tue Apr 11 16:00:51 PDT 2017; root:xnu-3248.60.11.5.3~1/RELEASE_X86_64 x86_64

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.

evocateur

Good start.

I'd really like to see an integration test to validate our usage of yargs.

evocateur · 2017-05-22T21:35:00Z

README.md

+Pass extra arguments to npm client.
+
+```sh
+$ lerna bootstrap --npm-client=yarn --npm-client-args=--ignore-engine


How do you pass multiple flags to --npm-client-args?

As --npm-client-args is just a string and simply proxies everything to underlying npm client, it'll be:

lerna bootstrap --npm-client=yarn --npm-client-args='--ignore-engine --flag1 --flag2 ...'

Unfortunately that's not how execa works, you need to pass an array of arguments (or push multiple, in this case) if there are multiple arguments.

evocateur · 2017-05-22T21:39:21Z

README.md

@@ -790,6 +790,24 @@ May also be configured in `lerna.json`:
 }
 ```

+#### --npm-client-args [client]


This needs to be defined for yargs in the builder export of BootstrapCommand.

Even better, why not just accept all args after a -- to lerna bootstrap to be passed to the client execution?

lerna bootstrap -- --ignore-engine --silent

To pass it via lerna.json, you'd need to pass an array of strings instead of a string:

"npmClientArgs": ["--ignore-engine", "--silent"]

Do we want to isolate it to bootstrap command only? I thought it should be for all of them regardless of where npm client is used.

bootstrap is currently the only place we alternate on the value of npmClient, and it seems cleaner that way (supporting multiple arguments properly without re-implementing yargs, for example)

It seems accepting all args after -- is the lottery winner. My only concern is it's not very explicit and bootstrap isn't just a proxy of [npm | yarn] install so it might be confusing whether args get appended to entire flow of bootstrap or particular parts of it.

I can live with that :)

@evocateur what if both npmClientArgs and -- are provided, should we append one to another or replace? I vote for append as it'll work like "common arguments you always want to pass".

xaka · 2017-05-23T00:01:57Z

@evocateur please take another look; i wanted to add integration tests, but couldn't find proper way to capture arguments and do assertion - feels like there is need for mock npm client for that

evocateur · 2017-05-23T00:06:49Z

It would be sufficient to test whether it respects --production --no-optional, for example, by asserting that a modules under devDependencies and optionalDependencies didn't get installed.

xaka · 2017-05-23T00:08:43Z

Good call. Will do.

evocateur · 2017-05-23T00:08:46Z

src/commands/BootstrapCommand.js

@@ -46,13 +46,19 @@ export default class BootstrapCommand extends Command {
  }

  initialize(callback) {
-    const { registry, npmClient } = this.options;
+    const { registry, npmClient, npmClientArgs } = this.options;


This needs a npm-client-args config in the builder object above

Specifically, using this block starting at line 12:

export function handler(argv) { return new BootstrapCommand([...argv.npmClientArgs], argv).run(); } export const command = "bootstrap [npm-client-args..]";

This pattern is used in lerna exec (and run): https://github.com/lerna/lerna/blob/master/src/commands/ExecCommand.js#L8

Thus you can alias a collection of all unparsed positional arguments, as well as unparsed remainders behind --.

lerna bootstrap --scope foo -- --bar lerna bootstrap --hoist -- --no-optional

It should still be retrievable from this.options.npmClientArgs, but it's also always present as an array (which might be empty).

evocateur · 2017-05-23T00:10:17Z

Also, don't worry about making several commits, I'll be squashing the PR anyway.

xaka · 2017-05-23T01:11:17Z

D'oh! Unfortunately we cannot move forward with yargs until there is fix for yargs/yargs#682. Next version (hasn't been released yet) adds populate-- option to change the behavior, but it's unclear when it'll become available. Until then it's difficult to distinguish between positional arguments and arguments that simply haven't been parsed.

xaka · 2017-05-23T01:51:59Z

@evocateur ready for another round with integration tests included! thanks for patience 🍻

xaka · 2017-05-23T02:00:00Z

test/integration/__snapshots__/lerna-bootstrap.test.js.snap

+  "packages/package-1/node_modules/clone",
+  "packages/package-1/node_modules/columnify",
+  "packages/package-1/node_modules/defaults",
+  "packages/package-1/node_modules/pify",


pify shouldn't be here, hmm

xaka · 2017-05-23T03:36:41Z

One test is failing and it says packages/package-1/node_modules/pify is expected to be installed, but when i manually run npm with same arguments, it doesn't happen. I'm confused...

xaka · 2017-05-23T04:43:34Z

Turned out it was lerna itself extracting and merging dependencies and devDependencies together, and passing it npm afterwards, which affected test at the end.

evocateur · 2017-05-23T05:04:21Z

Yeah, it's because pify is a transitive dependency of tempy, and thus will always be installed no matter the flags. I'd recommend choosing dependencies that don't have any dependencies themselves, so that the scope of the assertion is tightly focused.

normalize-newline, remove-trailing-separator, and balanced-match all fit the bill.

That being said, you're correct! We might want to focus on different flags, perhaps? Perhaps it would be easier to use --ignore-engines instead?

xaka · 2017-05-23T06:03:13Z

I ended up using fake npm client, which is less error prone, doesn't have side effects and comes with predictable behavior. Everything should stable now.

xaka · 2017-05-23T06:04:08Z

test/integration/lerna-bootstrap.test.js

 import normalizePath from "normalize-path";
+import path from "path";
+import tempy from "tempy";


I sorted everything as I didn't know where to put new imports

xaka · 2017-05-24T23:56:53Z

@evocateur knock knock!

DxCx · 2017-06-05T15:51:38Z

@xaka thanks alot!
@evocateur any expectation when this will be released?

evocateur

I think we're going to need to change part of the bootstrap algorithm to make this fully-functional. (the part where we mangle the temporary package.jsons)

evocateur · 2017-06-06T16:56:43Z

test/integration/lerna-bootstrap.test.js

@@ -65,6 +67,22 @@ describe("lerna bootstrap", () => {
      const stdout = await execa.stdout(LERNA_BIN, ["run", "test", "--", "--silent"], { cwd });
      expect(stdout).toMatchSnapshot("--npm-client yarn: stdout");
    });
+
+    test.concurrent("passes remaining arguments to npm client", async () => {
+      const cwd = await initFixture("BootstrapCommand/npm-client-args");


We also need an integration test that doesn't employ the npmClientArgs key in lerna.json (using the BootstrapCommand/basic fixture, for example). I'm pretty sure that fails right now, as is.

evocateur · 2017-06-06T16:58:19Z

test/fixtures/BootstrapCommand/npm-client-args/npm

@@ -0,0 +1,3 @@
+#!/usr/bin/env node
+
+require("fs").writeFileSync(require("path").resolve(__dirname, "npm-debug.log"), process.argv.slice(2));


I don't think we need to write a file, just console.log the argv and assert on the command's stdout.

unfortunately bootstrap doesn't proxy npm client output

evocateur · 2017-06-07T16:16:10Z

Of course, you're right! I apologize for my scattered attention.

…

On Jun 7, 2017, at 09:09, Pavel Strashkin ***@***.***> wrote: @xaka commented on this pull request. In test/fixtures/BootstrapCommand/npm-client-args/npm: > @@ -0,0 +1,3 @@ +#!/usr/bin/env node + +require("fs").writeFileSync(require("path").resolve(__dirname, "npm-debug.log"), process.argv.slice(2)); unfortunately bootstrap doesn't proxy npm client output — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

xaka · 2017-06-07T16:44:39Z

@evocateur added another integration test for remaining arguments without npmClientArgs in lerna.json

Timer · 2017-06-29T17:49:21Z

Hi! We'd love to see this merged for facebook/create-react-app#2673; is there anything I can do to help?

Specifically, we're trying to run yarn install --no-lockfile during bootstrap.

If not, take this as a friendly bump! 😄

evocateur · 2017-06-30T17:28:18Z

I didn't realize the durable npmClientArgs from lerna.json and any additional CLI flags would be merged, not overridden, so my previous comment about the config naming is invalid. Still wish the integration test was a little less "fake out npm", but it works, and that's the ultimate goal.

Thanks @xaka for your patience. :)

xaka · 2017-06-30T17:30:15Z

@evocateur just couple months, no prob man 🍻 😄 next time i'll start with FB to bribe you first, hehe

evocateur · 2017-06-30T17:32:19Z

Augh, AppVeyor is exasperating. 2 out of 3 pass, so I'm calling it good.

Timer · 2017-06-30T22:04:11Z

Cheers, thanks so much!

lock · 2018-12-27T13:10:28Z

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 May 22, 2017

View reviewed changes

xaka force-pushed the master branch 4 times, most recently from ec62e81 to e049d41 Compare May 23, 2017 00:00

evocateur reviewed May 23, 2017

View reviewed changes

xaka force-pushed the master branch from e049d41 to 06d9f76 Compare May 23, 2017 01:46

xaka commented May 23, 2017

View reviewed changes

xaka force-pushed the master branch 2 times, most recently from b1bec8c to 0304745 Compare May 23, 2017 03:27

xaka force-pushed the master branch 5 times, most recently from 765374b to 17a9bce Compare May 23, 2017 06:02

xaka commented May 23, 2017

View reviewed changes

evocateur suggested changes Jun 6, 2017

View reviewed changes

Allow to pass npmClient arguments (lerna#813)

7e504a2

xaka force-pushed the master branch from 17a9bce to 7e504a2 Compare June 7, 2017 16:39

Merge branch 'master' into master

e72c125

evocateur approved these changes Jun 30, 2017

View reviewed changes

evocateur added the Tag: Enhancement label Jun 30, 2017

evocateur changed the title ~~Allow to pass npmClient arguments (#813)~~ Pass extra arguments to npmClient during bootstrap Jun 30, 2017

evocateur merged commit 52ad617 into lerna:master Jun 30, 2017

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

		@@ -0,0 +1,3 @@
		#!/usr/bin/env node

		require("fs").writeFileSync(require("path").resolve(__dirname, "npm-debug.log"), process.argv.slice(2));

Pass extra arguments to npmClient during bootstrap #834

Pass extra arguments to npmClient during bootstrap #834

Conversation

xaka commented May 22, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

xaka commented May 23, 2017

evocateur commented May 23, 2017

xaka commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evocateur commented May 23, 2017

xaka commented May 23, 2017

xaka commented May 23, 2017

Choose a reason for hiding this comment

xaka commented May 23, 2017

xaka commented May 23, 2017

evocateur commented May 23, 2017

xaka commented May 23, 2017

Choose a reason for hiding this comment

xaka commented May 24, 2017

DxCx commented Jun 5, 2017

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

evocateur commented Jun 7, 2017 via email

xaka commented Jun 7, 2017

Timer commented Jun 29, 2017 • edited Loading

evocateur commented Jun 30, 2017

xaka commented Jun 30, 2017

evocateur commented Jun 30, 2017

Timer commented Jun 30, 2017

lock bot commented Dec 27, 2018

xaka commented May 22, 2017 •

edited

Loading

Timer commented Jun 29, 2017 •

edited

Loading