enabled is global state

[keis]

Using chalk from a library I know I always want colours enabled as it is explicitly requested to begin with. But doing require('chalk').enabled = true would be bad pratice as this enforce the setting globally and possibly interfere with other libraries using chalk and depending on the auto-detection.

It would be useful to be able to set this option in some isolated way.

[sindresorhus]

Chalk is a singleton by design. You either want colors outputted or you don't. This doesn't interfere with other libraries as they need to handle (usually not do anything special) both enabled and not anyways.

// @jbnicolai

[keis]

ok, that's too bad. This means chalk is not really usable for adding colour
escapes to strings that are not immediately for used for terminal output.

[sindresorhus]

This means chalk is not really usable for adding colour
escapes to strings that are not immediately for used for terminal output.

What do you need that for?

[keis]

In my case it's a logger for which you can customise the colours for each stream: file, stderr and what not. I don't really want to second guess the user and disable colours after they specifically requested it.

However using this in a application that logs to stderr and usually have stdout redirected, colours gets disabled and I get boring text in my terminal.

But at the same time I don't want to force it on in case someone is actually printing coloured stuff to stdout outside the logger and expect the auto-detect bahaviour.

[sindresorhus]

Hmm, I hadn't really thought of this kind of use-case. I'll need some time to think about this.

@jbnicolai Thoughts?

[julien-f]

require('chalk') could be a factory which creates chalk instance and also be the default global instance.

Current usage would be the same and @keis could call it to get its own instance:

var chalk = require('chalk')();
chalk.enabled = true;

[UltCombo]

Turning the main export into a factory would be the proper fix I believe, but that means breaking back-compat.

[stevenvachon]

Why not make chalk optionally instance-based?

var chalk = require("chalk").instance(); // or .new() if it's allowed
chalk.enabled = true; // only in this module

I had a similar concern with one of my libraries and just opted for forced instantiation via new, but because my lib is not very popular I was free to change it.

---

Upon second glance, this is barely any different from the "factory" suggestion above. I suck with technical terms.

[sindresorhus]

I'm leaning towards making the default export be a factory. I'm ok with breaking backwards compat. Now is the time to do it.

Is there any downsides with making it a factory that I'm not seeing? Like, would it impact performance?

[naartjie]

I think making a backwards incompatible change would be bad form, since it's possible to keep the current interface (this is clearly the majority of chalk's use cases), as well as allow for @keis's use case (which is an edge case).

@julien-f this way is very cool and compact, but it's not clear what it does when looking at the code, plus if someone logs an undefined with their default instance, it returns a new instance instead of the output? I'm sure there are ways around this, but for me this is too much trickery.

var chalkDefault = require('chalk');
var chalkAdditional = require('chalk')();

@stevenvachon's suggestion is more explicit and clean in what it does. It is longer but obviously most of your users won't be using it this way. My vote goes to this one:

var chalkDefault = require('chalk');
var chalkAdditional = require('chalk').new() // or .instance() as suggested, or newInstance();

// then somewhere in your code you could
chalkAdditional.enabled = false; // leaving the default enabled

[UltCombo]

So require('chalk') returns an instance, and instances have a new method which returns a new instance. Looks clean enough and still back-compatible.

[SBoudrias]

FWIW, that's the solution I came up with Inquirer:

var inquirer = require('inquirer');
var independantInquirer = require('inquirer').createPromptModule();

[sindresorhus]

since it's possible to keep the current interface (this is clearly the majority of chalk's use cases)

But does it really make sense to support globally disabling chalk? I thought it was a good idea when I implemented it for end-users to turn off colors, but it's becoming clear that it can easily be abused by some random module. And if you really want to do it you can process.argv.push('--no-color').

[naartjie]

But does it really make sense to support globally disabling chalk? I thought it was a good idea when I implemented it for end-users to turn off colors, but it's becoming clear that it can easily be abused

Good point.

How about just ignoring enabled in the default instance, thus:

require('chalk').enabled = false;

would have no effect, and on top of documenting it, chalk could spit out a warning/deprecated message if someone tries to set this property on the default instance.

And then:

var chalkAdditional = require('chalk').instance();
chalkAdditional.enabled = false;

would disable only that instance. This could work, no?

[sindresorhus]

I want as simple interface as possible. That's why I'm asking if ability to disable globally is really something that should stay? Otherwise I can just go with instance based by default and have a simpler API.

[naartjie]

I'm asking if ability to disable globally is really something that should stay?

IMO, no. Given the points above, you should not be able to disable on a global level.

[naartjie]

@sindresorhus the end-users will still need some way to disable globally, so maybe you should leave the ability to disable through something like process.argv.push('--no-color')... Although it means it could still be abused :-/

[stevenvachon]

enabled is also useful for enabling colours in non-tty situations where its value would be false as a result of the supports-color module.

[naartjie]

enabling colours in non-tty situations

That's true. Darnit, since having enabled = true be effective, and ignoring enabled = false doesn't make sense, I say enabled be left alone as is on the default instance. #changeofvote

[keis]

I think keeping the global enabled for backwards compability is fine as long as you can create a new context which is unaffected by changes to the global instance.

If structuring the module so that you do 'module.exports = new Chalk();' to create the default context, thennew require('chalk').constructor() is a possible syntax that would require no special factory method.

[jbnicolai]

I agree that overwriting the globally shared enabled state is begging for issues when dependencies use Chalk as well. Breaking compatibility is going to cause issues for a lot of people though (remember: 2200+ dependants).

I suggest we go with adding a function to the global Chalk object to create a new, not-shared instance. The syntax @keis proposed seems nice.