Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FORCE_COLOR=0 forces color #43

Open
chocolateboy opened this issue Jun 22, 2020 · 11 comments
Open

FORCE_COLOR=0 forces color #43

chocolateboy opened this issue Jun 22, 2020 · 11 comments

Comments

@chocolateboy
Copy link
Contributor

Re-opening this to ensure it doesn't get overlooked. Test case copied from #41 (comment). Fixed in #41, but the patch was modified, which introduced the new bug.

In the current version (1.2.0) of Colorette, the following all have the same effect:

  • FORCE_COLOR=1
  • FORCE_COLOR=0
  • FORCE_COLOR=

i.e. FORCE_COLOR=<falsey/no value> incorrectly forces color which wouldn't otherwise be displayed.

test.js

const { green } = require('colorette')

console.log(green('Hello, world!'))

1) expect color

$ FORCE_COLOR=1 node ./test.js | strings

output ✔

[32mHello, world!
[39m

2) expect no color (because of the pipe)

$ node ./test.js | strings

output ✔

Hello, world!

3) expect no color (same as 2)

$ FORCE_COLOR=0 node ./test.js | strings

output ❌ 🐛

[32mHello, world!
[39m

4) expect no color (same as 2)

$ FORCE_COLOR= node ./test.js | strings

output ❌ 🐛

[32mHello, world!
[39m
@jorgebucaran
Copy link
Owner

Thank you, @chocolateboy. I recently added tests for FORCE_COLOR and NO_COLOR.

Could you explain why FORCE_COLOR=0 node ./test.js | strings or FORCE_COLOR= node ./test.js | strings expects no color?

I thought I had understood the issue, but I think I lost it again. My understanding is that FORCE_COLOR={1,2,3...} forces color and that FORCE_COLOR={,0} should do nothing (as opposed to deliberately disable color).

@jorgebucaran
Copy link
Owner

To answer my own question, I think it is because when we pipe colorette, color should be disabled.

@chocolateboy
Copy link
Contributor Author

The "no color" in those cases is because of the pipe. The FORCE_COLOR is ignored in those cases i.e. it doesn't/shouldn't force color on or force color off. (It's kind of a red("herring") :-)

@jorgebucaran
Copy link
Owner

🎉 @chocolateboy I got it! But...


Now that I finally understand the issue, I'd like to advocate against implementing FORCE_COLOR=0 as described in this comment. Why? I think presence of FORCE_COLOR in process.env, regardless of its value, should force color. But why? For consistency with its evil twin NO_COLOR which we also support.

All command-line software which outputs text with ANSI color added should check for the presence of a NO_COLOR environment variable that, when present (regardless of its value), prevents the addition of ANSI color.

It's also easier to reason about FORCE_COLOR and NO_COLOR as boolean pairs, rather than using FORCE_COLOR=0 to mean don't force color, which I find super cryptic. And if we went that route, I guess we'd also have to implement NO_COLOR=0. But what does that even mean? My brain exploded while trying to parse that. 😄

Now, I get it, maybe you have a script where you set FORCE_COLOR's value. Well, instead of switching from a 0 to a 1 (or was it 1 to 0?) use the right variable.

Here are two ways to do it that come to mind:

I use the fish shell and we need to place env before declaring environment variables, but you should be able to figure out the equivalent in your own shell.

set -l COLOR_MODE = NO_COLOR # or FORCE_COLOR

env $COLOR_MODE= ./test.js

or a one-liner:

env (test $condition && echo FORCE_COLOR || echo NO_COLOR)= ./test.js

@chocolateboy
Copy link
Contributor Author

chocolateboy commented Jun 26, 2020

I think this is an awful lot of work to avoid adding a few bytes to the source code :-/

As you mention, the behavior of NO_COLOR= and NO_COLOR=0 (both equivalent to NO_COLOR=1) is non-standard and evil. No other environment variables behave that way.

It's also easier to reason about FORCE_COLOR and NO_COLOR as boolean pairs

I think it's easy enough to reason about the semantics if the code is written clearly, which is why I ungolfed it. Either way, as mentioned before:

I don't think anything else interprets FORCE_COLOR in this way (i.e. FORCE_COLOR=0 forces color), not even chalk, which supports FORCE_COLOR=0, FORCE_COLOR=1, FORCE_COLOR=2 and FORCE_COLOR=3 (!).

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 26, 2020

@chocolateboy I respect your opinion, but I don't consider that to be a lot of work. And that's only work you'd do only if you wanted to switch between FORCE_COLOR and NO_COLOR (I don't think anyone would normally do that).

Frankly, I don't care about the bytes, this is Node after all—your node_modules are heavier than the universe. My reason to go against this is not bytes. I just find things like FORCE_COLOR=0 or FORCE_COLOR=1 to be confusing. And I didn't mean NO_COLOR is literally evil, haha, that was just a way to refer to NO_COLOR and FORCE_COLOR as counterparts.

I don't think there's a standard governing any of this, so I wouldn't claim the behavior of NO_COLOR to be non-standard. But I'm not an expert either. It's what it is, at least they have a website https://no-color.org.


I don't think anything else interprets FORCE_COLOR in this way (i.e. FORCE_COLOR=0 forces color), not even chalk, which supports FORCE_COLOR=0, FORCE_COLOR=1, FORCE_COLOR=2 and FORCE_COLOR=3 (!).

My eyes!!! 👀🩸

When I first encountered chalk way back in the day, I was frustrated by its complexity. Today, it's even more complex. It's complex any way you look at it. Some people are more adept at dealing with complexity. Others don't care. I am very particular about this. I just want colorette to be simple, whether you are using it or looking under the covers. It was probably already a bad idea enabling/disabling color out of the box, but I can't roll that back now.

@chocolateboy
Copy link
Contributor Author

AFAICT, no other JS implementations of FORCE_COLOR behave in the way you're proposing (i.e. like NO_COLOR), and only NO_COLOR (mis)behaves in the way you're proposing, so I don't think adopting these two non-standards makes things simpler or easier to reason about.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jun 27, 2020

@chocolateboy What if we could do better than them? By the way, do you have an example of FORCE_COLOR used in the wild?

@chocolateboy
Copy link
Contributor Author

chocolateboy commented Jun 27, 2020

What if we could do better than them?

I don't think "everyone else is doing it wrong - let's innovate!" is a better approach. FORCE_COLOR may not have its own website, but it's still an informal standard which no other JS library I can find interprets in the way you're proposing. Yes, FORCE_COLOR=0 may be interpreted in two different ways, but I can't see how introducing a new, third way is an improvement.

do you have an example of FORCE_COLOR used in the wild?

My original use case is here:

I'm trying to pipe colored output through a shell pipeline in a TTY, but setting FORCE_COLOR to a truthy value doesn't seem to work

If you mean an example of how it's interpreted in Node.js libraries/apps, see here. I couldn't find an example in the first 10 pages which behaves in the way you're proposing (i.e. like NO_COLOR), but someone with more time may be able to find one.

@nicolo-ribaudo
Copy link

Hey sorry to comment on an old issue, but Node.js itself now disables color then FORCE_COLOR is set to 0: https://nodejs.org/api/tty.html#writestreamgetcolordepthenv

It'd be great for JS libraries to align with the Node.js behavior.

@jorgebucaran
Copy link
Owner

Thanks for the heads-up, @nicolo-ribaudo! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants