Allow Node arguments to be configured #2272

maximelkin · 2019-10-17T00:09:50Z

Fixes #2090

IssueHunt Summary

Referenced issues

This pull request has been submitted to:

#2090: Add a --node-arguments flags

IssueHunt has been backed by the following sponsors. Become a sponsor

novemberborn · 2019-11-03T16:39:47Z

Hey @maximelkin sorry about the delay in reviewing. At first glance this looks great, though I'd like to play with it a little first.

novemberborn

This is really nice @maximelkin! I kept writing comments and then looking at other code and realizing I got things wrong… by which I mean to say you really did think through all the angles here.

I've pushed some small tweaks, and left some other comments as well. Let me know what you think.

lib/api.js

lib/node-arguments.js

docs/05-command-line.md

…stances

maximelkin · 2019-11-24T23:55:19Z

Not sure, why tests getting SIGKILL on local machine

novemberborn

Thanks for your continuing work on this @maximelkin. I think it's nearly there.

Hopefully I've figured out how to make the CI run on pull requests as well.

novemberborn · 2019-12-01T15:42:24Z

lib/api.js

@@ -15,6 +16,8 @@ const makeDir = require('make-dir');
 const ms = require('ms');
 const chunkd = require('chunkd');
 const Emittery = require('emittery');
+const minimist = require('minimist');


Could we use https://www.npmjs.com/package/yargs-parser instead? It's already in our dependency tree as part of meow.

(Though it looks like so is minimist, because Node.js modules, but for less apparent reasons.)

I choose minimist, because it has reverse package, maybe there is another package for it?

I reckon dargs may work with yargs-parser too, assuming it also outputs straight-forward objects.

novemberborn · 2019-12-01T15:43:54Z

lib/api.js

+		const hasInspect = args.inspect || args['inspect-brk'];
+		if (hasInspect) {
+			if (args.inspect && args['inspect-brk']) {
+				throw new Error('Flags inspect and inspect-brk provided simultaneously');


Note to self: check how this ends up being reported.

novemberborn · 2019-12-01T15:47:26Z

lib/cli.js

@@ -115,6 +116,10 @@ exports.run = async () => { // eslint-disable-line complexity
 				type: 'boolean',
 				default: false
 			},
+			'node-arguments': {
+				type: 'string',
+				default: ''


Can we remove this default? See my comment for normalizeNodeArguments().

novemberborn · 2019-12-01T15:48:24Z

lib/node-arguments.js

+ * @param {string} cliParams
+ * @return {object}
+ */
+function normalizeNodeArguments(confParams, cliParams) {


If we can assume the arguments may be undefined, we can use default parameter values:

Suggested change

function normalizeNodeArguments(confParams, cliParams) {

function normalizeNodeArguments(confParams = [], cliParams = '') {

And we can get rid of the JSDoc comment too.

We do need to enforce confParams is an array.

novemberborn · 2019-12-01T15:49:08Z

lib/node-arguments.js

+function normalizeNodeArguments(confParams, cliParams) {
+	return omit({
+		...minimist(confParams || []),
+		...minimist(cliParams.split(' '))


Are there Node.js arguments that take strings? Cause then splitting on the empty string won't work.

novemberborn · 2019-12-01T15:50:20Z

lib/node-arguments.js

+	return omit({
+		...minimist(confParams || []),
+		...minimist(cliParams.split(' '))
+	}, '_');


Is the _ returned by minimist? I'd prefer rest-spread for this:

const { _, ...normalized } = { ...minimist(), ...minimist() }; return normalized;

novemberborn · 2019-12-01T15:54:43Z

lib/api.js

-		if (inspectArgIndex === -1) {
-			return Promise.resolve(execArgv);
-		}
+		const mainProcessArgs = minimist(process.execArgv);


Could we cache this? This function runs for every test file.

Not sure, what I correctly understand you, check please

niftylettuce · 2019-12-05T20:02:58Z

Great work! Will be so nice to have since Node v12 changed TLS min version and we need --tls-min-v1 flag as a Node arg for a project of mine.

novemberborn · 2019-12-08T16:09:09Z

Hi @maximelkin, I did not have time to look at your changes yet, sorry.

maximelkin · 2019-12-08T22:00:52Z

About passing arguments via cli
Maybe it's better to pass as --node-arguments "a b c"?
As I see this is supported by yargs-parser (and meow) now

novemberborn · 2019-12-21T13:08:35Z

About passing arguments via cli
Maybe it's better to pass as --node-arguments "a b c"?
As I see this is supported by yargs-parser (and meow) now

@maximelkin what do you mean by this?

I spotted the change of -r to --require in the CLI helper. This led me of on a chase as to why -r wasn't working. It's because with the default configuration, dargs outputs -r=foo.js which Node.js does not support.

This then led me to think about how we're combining the arguments. I think we should simplify it a little:

Take process.execArgv
Concatenate with the nodeArguments array from the AVA config
Concatenate with the value of --node-arguments passed to AVA on the CLI
Provide this combined array as the execArgv option when forking the child process

This without parsing into objects, though we do need to parse the value to the --node-arguments option into the proper array.

I guess what may complicate this is the dynamic port assignment when we see --inspect flags. I'm hoping to do a bunch of work on AVA over the next two weeks, so maybe I can get rid of that too.

novemberborn · 2019-12-23T16:55:39Z

I've switched the CLI flag parser to yargs (#2315) which will cause some conflicts here. But, #2316 adds a built-in debug command, which simplifies the execArgv stuff in api.js.

novemberborn · 2020-01-06T08:43:40Z

@maximelkin I've been making a lot of changes over the past few weeks which has caused a lot of merge conflicts here. Sorry about that. I can try and resolve them when I get some time.

With regards to parsing --node-arguments, I was thinking we could maybe launch a script as a child process which outputs console.log(JSON.stringify(process.execArgv)). That way we can use Node.js own argument parsing logic.

* Combine values without overwriting duplicates * Use Node.js to parse the argv string Also add a bit more context in the documentation.

novemberborn · 2020-01-12T15:19:28Z

@maximelkin I've fixed the merge conflicts and simplified the implementation as per #2272 (comment). Please let me know what you think!

novemberborn · 2020-01-12T15:33:50Z

I was afraid this wouldn't work on Windows though 😢

maximelkin · 2020-01-14T19:07:55Z

Hello, sorry for long inactivity
All is ok, very interesting hack with calling node for parsing
Also, maybe ask nodejs to expose algorithm of doing this? (for better implementation in future)

maximelkin · 2020-01-14T21:58:54Z

So adding shell: true to spawn test make it works, but break watcher test
Watcher breaking seems to be suspicious for me, I will research this

novemberborn · 2020-01-18T13:14:43Z

All is ok, very interesting hack with calling node for parsing
Also, maybe ask nodejs to expose algorithm of doing this? (for better implementation in future)

I found https://github.com/astur/arrgv which claims to work just like Node.js does. Looking at the implementation in Node.js itself it also does a bunch of validation, which could lead to unexpected errors. So a simple parser is better. And this one gives us arrays which is what we want. Plus, it's tested using AVA 😄

Let's see if CI passes and then let's ship this!

maximelkin changed the title ~~add node-arguments flag~~ [WIP]: add node-arguments flag Oct 17, 2019

maximelkin force-pushed the node-arguments branch 2 times, most recently from ec5304d to 1f81b2f Compare October 17, 2019 00:34

maximelkin changed the title ~~[WIP]: add node-arguments flag~~ add node-arguments flag Oct 17, 2019

novemberborn reviewed Nov 10, 2019

View reviewed changes

lib/api.js Outdated Show resolved Hide resolved

lib/node-arguments.js Outdated Show resolved Hide resolved

lib/node-arguments.js Outdated Show resolved Hide resolved

lib/node-arguments.js Outdated Show resolved Hide resolved

docs/05-command-line.md Outdated Show resolved Hide resolved

novemberborn mentioned this pull request Nov 10, 2019

The ESM Plan #2293

Closed

12 tasks

maximelkin force-pushed the node-arguments branch from 15d1e19 to 5998865 Compare November 24, 2019 23:06

maxeljkin and others added 6 commits November 25, 2019 02:07

feat: node-arguments for passing arguments to child processes node in…

103a243

…stances

Remove unnecessary promise-wrapping

0cc0abd

Destructuring

3f6633c

Tweak docs

ae70f22

node-arguments: extend cli doc with example

420c9b7

node-arguments simplify processing

8e51406

maximelkin force-pushed the node-arguments branch from 5998865 to 8e51406 Compare November 24, 2019 23:07

Maksim Elkin added 3 commits November 25, 2019 02:35

fixes

d4acaeb

fix(node-arguments): default value for config from file value

adfc212

fix: node-arguments test fix

26c4448

maximelkin requested a review from novemberborn November 24, 2019 23:55

novemberborn added 2 commits December 1, 2019 16:52

Clarify CLI use of --node-arguments

5b99832

Merge branch 'master' into node-arguments

9f00c05

novemberborn requested changes Dec 1, 2019

View reviewed changes

Maksim Elkin added 6 commits December 7, 2019 13:28

fix normalize arguments

5cf8dd7

fix all other minimist usages

28a2c06

fix semicolon

1ae69c4

fix broken tests?

c1a99d4

fix configuration for yargs parsing

8e649b9

memoize parse args

e00e80f

Maksim Elkin added 2 commits December 7, 2019 14:56

fix lint

ae3836b

move _parseProcessArgs to top

be272c7

remove node-arguments default for cli

b5d2796

Merge branch 'master' into node-arguments

18b7074

novemberborn approved these changes Jan 12, 2020

View reviewed changes

Simplify implementation

037cc33

* Combine values without overwriting duplicates * Use Node.js to parse the argv string Also add a bit more context in the documentation.

novemberborn force-pushed the node-arguments branch from f1d24ad to 037cc33 Compare January 12, 2020 15:21

Maksim Elkin added 7 commits January 14, 2020 22:36

update lockfile to master

518c44d

remove redundant \\

7aad65a

fix quotes

0e21309

stdout/stderr logging for node-arguments test

c40194a

fix format

f01a038

add stdout/stderr logs to another test

438148c

fix node-arguments integration test on windows

f670481

novemberborn added 2 commits January 18, 2020 14:03

Merge branch 'master' into node-arguments

7ec7f34

Parse using arrgv

40e7716

novemberborn changed the title ~~add node-arguments flag~~ Allow Node arguments to be configured Jan 18, 2020

novemberborn merged commit 7b20f6c into avajs:master Jan 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Node arguments to be configured #2272

Allow Node arguments to be configured #2272

maximelkin commented Oct 17, 2019 •

edited by issuehunt-oss bot

Loading

novemberborn commented Nov 3, 2019

novemberborn left a comment

maximelkin commented Nov 24, 2019

novemberborn left a comment

novemberborn Dec 1, 2019

maximelkin Dec 2, 2019

novemberborn Dec 3, 2019

novemberborn Dec 1, 2019

novemberborn Dec 1, 2019

novemberborn Dec 1, 2019

novemberborn Dec 1, 2019

novemberborn Dec 1, 2019

novemberborn Dec 1, 2019

maximelkin Dec 7, 2019

niftylettuce commented Dec 5, 2019

novemberborn commented Dec 8, 2019

maximelkin commented Dec 8, 2019

novemberborn commented Dec 21, 2019

novemberborn commented Dec 23, 2019

novemberborn commented Jan 6, 2020

novemberborn commented Jan 12, 2020

novemberborn commented Jan 12, 2020

maximelkin commented Jan 14, 2020

maximelkin commented Jan 14, 2020 •

edited

Loading

novemberborn commented Jan 18, 2020

	function normalizeNodeArguments(confParams, cliParams) {
	function normalizeNodeArguments(confParams = [], cliParams = '') {

Allow Node arguments to be configured #2272

Allow Node arguments to be configured #2272

Conversation

maximelkin commented Oct 17, 2019 • edited by issuehunt-oss bot Loading

Referenced issues

novemberborn commented Nov 3, 2019

novemberborn left a comment

Choose a reason for hiding this comment

maximelkin commented Nov 24, 2019

novemberborn 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

Choose a reason for hiding this comment

niftylettuce commented Dec 5, 2019

novemberborn commented Dec 8, 2019

maximelkin commented Dec 8, 2019

novemberborn commented Dec 21, 2019

novemberborn commented Dec 23, 2019

novemberborn commented Jan 6, 2020

novemberborn commented Jan 12, 2020

novemberborn commented Jan 12, 2020

maximelkin commented Jan 14, 2020

maximelkin commented Jan 14, 2020 • edited Loading

novemberborn commented Jan 18, 2020

maximelkin commented Oct 17, 2019 •

edited by issuehunt-oss bot

Loading

maximelkin commented Jan 14, 2020 •

edited

Loading