Only forward `color` flag to worker if supplied by user #1401

kevva · 2017-06-04T18:01:56Z

Didn't bother to add tests since we already have ones in place for this here. Also removed a test that is moot now.

novemberborn · 2017-06-09T13:06:48Z

process.argv is still a little different from what users might expect. Should we splice out the options?

I took #1393 (comment) to read us having consensus on only forwarding --color and --no-color. Previously we've argued that users should use environment variables to pass runtime values to their tests. These flags don't sit well with that though.

Does it make sense to pass AVA's command line arguments? Should we instead look for a -- argument and forward everything after that? But then does that imply users need to do ava --color -- --color if they want colors from AVA and whatever code runs in their tests? OTOH that would make it possible to do ava --color -- --no-color.

@sindresorhus?

sindresorhus · 2017-06-18T17:31:12Z

Should we splice out the options?

👍

I took #1393 (comment) to read us having consensus on only forwarding --color and --no-color. Previously we've argued that users should use environment variables to pass runtime values to their tests. These flags don't sit well with that though.

Yes, we should forward them if they're specified by the user. It's an exception since the flags apply to both us and other modules. ava --color -- --color would be super weird. I do not want that.

OTOH that would make it possible to do ava --color -- --no-color.

I don't see why you would want to do that. Colors is a binary thing. Either you want them or you don't. I don't think we should complicate it more.

sindresorhus · 2017-06-18T17:33:22Z

@kevva You need to handle the color option in the worker, by setting the Chalk singleton .enabled property to the same value as the .enabled property of Chalk in the CLI process. So the automatic color support is brought into the child worker, which defaults to no colors always because it's not a TTY.

duartealexf · 2017-07-11T02:56:35Z

Is this going to be merged anytime soon? Please, I really need to pass some arguments to my tests :)

novemberborn · 2017-07-12T10:51:35Z

@duartealexf the PR is not ready yet. If you could take @kevva's commit and iterate on it in a new PR then we could get it to a state where we can land it. I'm sure he won't mind.

kevva · 2017-07-13T18:00:21Z

@novemberborn, I'm going to address your comments soon :).

novemberborn · 2017-07-13T20:59:07Z

Sorry @duartealexf, @kevva is still on the case :)

Fixes #1393.

kevva · 2017-09-13T19:56:49Z

Ok, so I've changed the PR to only forward the color/no-color flag if it's set, and I'm also setting chalk.enabled in the worker.

Should we include support for different levels as in #1455 too?

kevva · 2017-09-13T20:06:00Z

Some tests are failing now because they relied upon checking the ava options forwarded as arguments before.

novemberborn

Should we include support for different levels as in #1455 too?

That'd be good, but we can do it as a follow-up.

novemberborn · 2017-09-17T14:12:17Z

lib/fork.js

+const args = [JSON.stringify(opts)];
+
+if (hasFlag('--color') || hasFlag('--no-color')) {
+args.push(`--color=${opts.color}`);


Shouldn't this forward --no-color if specified? And forward --color exactly as it was received?

To forward them exactly like they were received requires some sort of parsing. We can forward --no-color instead of --color=false if it was specified though.

novemberborn · 2017-09-17T14:12:43Z

lib/test-worker.js

 const serializeError = require('./serialize-error');

+// Initialize color support
+chalk.enabled = typeof opts.color === 'boolean' && opts.color;


opts.color already is a boolean, courtesy of

ava/lib/cli.js

Line 78 in 1df502d

color: 'color' in conf ? conf.color : require('supports-color') !== false,

.

Yeah, I did this just to make this test pass since it doesn't run the CLI.

That's an odd test. Just remove it 😄

novemberborn

👍 on passing --no-color as is.

I can't figure out what to do with --color, and I'm continually confused about it 😭

My take, if --color is set, it should be forwarded in exactly the same way. If it's not set, it shouldn't be forwarded.

But then how do we forward the actual color support to the workers? Do we inject it into chalk / support-color? Or really should we always set --color regardless of how AVA is called (unless of course --no-color was used)?

I think I'm fine with doing that, but it goes against the idea of AVA forwarding arguments as-is…

novemberborn · 2017-09-24T17:14:11Z

lib/fork.js

+}
+
+if (hasFlag('--no-color')) {
+args.push(`--no-color`);


Although XO doesn't complain, I'd have expected a regular string here, not a templated one.

Oh, escaped my eyes.

kevva · 2017-09-24T22:35:45Z

My take, if --color is set, it should be forwarded in exactly the same way. If it's not set, it shouldn't be forwarded.

Reason I didn't implement it that way is because it would've required some sort of command line arguments parsing, and it wouldn't look too good using meow in there. I extracted the command line parsing out of meow though, and created an option that I think would fix this use case https://github.com/kevva/argvments#any.

novemberborn · 2017-10-01T15:10:47Z

Reason I didn't implement it that way is because it would've required some sort of command line arguments parsing, and it wouldn't look too good using meow in there. I extracted the command line parsing out of meow though, and created an option that I think would fix this use case kevva/argvments#any.

Nice! So we could do the following:

	No flags	`--color`	`--color=mode`	`--no-color`
Set flag on worker process	No	Yes, as is	Yes, as is	Yes, as is
AVA configures `chalk` directly	Yes, with supported mode from main process	Yes, with supported mode from main process	No	No

(I'm not sure now whether AVA configures chalk or another package.)

@kevva does this strike the right balance between forwarding color mode by default and not behaving against the user's intention when --color or --no-color are provided?

And do we need to handle FORCE_COLOR environment variables?

novemberborn · 2017-10-26T14:32:10Z

@kevva any thoughts on my last comment?

@nowells does my proposal satisfy your concerns raised in #1455?

novemberborn · 2018-02-12T14:07:10Z

#1700 clears up what arguments are exposed to the test worker. Let's discuss the specifics around configuring color in #1701.

sindresorhus force-pushed the master branch from bbedc8f to 2360256 Compare June 27, 2017 21:45

novemberborn mentioned this pull request Jul 20, 2017

Do not clobber forked process color level #1455

Closed

kevva added 2 commits September 13, 2017 21:52

Run workers with user supplied command line arguments

7bfc5aa

Fixes #1393.

Only pass color flag to worker and remove internal options

6be4470

kevva force-pushed the remove-flags branch from 2249a3b to 6be4470 Compare September 13, 2017 19:53

kevva changed the title ~~Run workers with user supplied command line arguments~~ Only forward color flag to worker if supplied by user Sep 13, 2017

novemberborn reviewed Sep 17, 2017

View reviewed changes

novemberborn mentioned this pull request Sep 17, 2017

Stabilize colors #1524

Merged

Pass --no-color as is

889d87f

novemberborn requested changes Sep 24, 2017

View reviewed changes

Don't unnecessarily use template literals

9296ddf

novemberborn mentioned this pull request Feb 12, 2018

Ensure workers inherit color level from main process #1701

Closed

novemberborn closed this Feb 12, 2018

sindresorhus deleted the remove-flags branch February 13, 2018 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Only forward `color` flag to worker if supplied by user #1401

Only forward `color` flag to worker if supplied by user #1401

Uh oh!

kevva commented Jun 4, 2017

novemberborn commented Jun 9, 2017

sindresorhus commented Jun 18, 2017

sindresorhus commented Jun 18, 2017

duartealexf commented Jul 11, 2017

novemberborn commented Jul 12, 2017

kevva commented Jul 13, 2017

novemberborn commented Jul 13, 2017

kevva commented Sep 13, 2017 •

edited

Loading

kevva commented Sep 13, 2017

novemberborn left a comment

novemberborn Sep 17, 2017

kevva Sep 18, 2017

novemberborn Sep 17, 2017

kevva Sep 17, 2017

novemberborn Sep 17, 2017

novemberborn left a comment

novemberborn Sep 24, 2017

kevva Sep 24, 2017

kevva commented Sep 24, 2017

novemberborn commented Oct 1, 2017

novemberborn commented Oct 26, 2017

novemberborn commented Feb 12, 2018

Labels

5 participants

Only forward color flag to worker if supplied by user #1401

Only forward color flag to worker if supplied by user #1401

Uh oh!

Conversation

kevva commented Jun 4, 2017

novemberborn commented Jun 9, 2017

sindresorhus commented Jun 18, 2017

sindresorhus commented Jun 18, 2017

duartealexf commented Jul 11, 2017

novemberborn commented Jul 12, 2017

kevva commented Jul 13, 2017

novemberborn commented Jul 13, 2017

kevva commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

kevva commented Sep 13, 2017

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Sep 17, 2017

Choose a reason for hiding this comment

kevva Sep 18, 2017

Choose a reason for hiding this comment

novemberborn Sep 17, 2017

Choose a reason for hiding this comment

kevva Sep 17, 2017

Choose a reason for hiding this comment

novemberborn Sep 17, 2017

Choose a reason for hiding this comment

novemberborn left a comment

Choose a reason for hiding this comment

novemberborn Sep 24, 2017

Choose a reason for hiding this comment

kevva Sep 24, 2017

Choose a reason for hiding this comment

kevva commented Sep 24, 2017

novemberborn commented Oct 1, 2017

novemberborn commented Oct 26, 2017

novemberborn commented Feb 12, 2018

Labels

5 participants

Only forward `color` flag to worker if supplied by user #1401

Only forward `color` flag to worker if supplied by user #1401

kevva commented Sep 13, 2017 •

edited

Loading