lukeed/kleur

Enabled should be determined on demand

jaydenseric opened this issue · 2 comments

Ideally, the environment should be checked for color support every time kleur is used, because environment variables can be changed at runtime. At the moment, the enabled property is determined once at module load time:

kleur/index.mjs

Lines 3 to 12 in fc9cc25

let FORCE_COLOR, NODE_DISABLE_COLORS, NO_COLOR, TERM, isTTY=true;
if (typeof process !== 'undefined') {
({ FORCE_COLOR, NODE_DISABLE_COLORS, NO_COLOR, TERM } = process.env);
isTTY = process.stdout && process.stdout.isTTY;
}
const $ = {
enabled: !NODE_DISABLE_COLORS && NO_COLOR == null && TERM !== 'dumb' && (
FORCE_COLOR != null && FORCE_COLOR !== '0' || isTTY
),

We should be able to set process.env.FORCE_COLOR = '1', run a snapshot test of some colorized output, then revert the FORCE_COLOR environment variable back to what it was again. At the moment doing this causes snapshot tests to fail in a GitHub Actions CI environment, as the process is not a TTY when the kleur module is loaded causing kleur.enabled to be false and setting FORCE_COLOR to 1 at runtime makes no difference prior to calling kleur.

As side comment, the current API that encourages people to modify an enabled property of the module that affects every use of kaleur in your project and node_modules is not really acceptable (kind of related: #34). An ideal API would be something like this:

const {
  kleur,

  // The default function, in case a user wants to use it as a base.
  checkEnabled,
} = require('kleur');

const formatter = kluer({
  // Runs when kleur is used. Returns a boolean, with potentially
  // complex logic reading the env, etc.
  checkEnabled: () => true,
})

const message = formatter.bold().red('Foo!');

A user in a rush can just do:

const message = kluer().bold().red('Foo!');

Set the ENV variable before requiring kleur.

Kleur is meant to respond to the environment -- as do most colorizers, even in other languages. There are no plans to export a factory. IMO that's a far worse API given the 99% use case.

Sorry, but hope that helps explain things a little bit.