Switch from chalk to colorette

[kibertoad]

fixes #208

[kibertoad]

Needs work

[kibertoad]

@jorgebucaran Any ideas why this might be failing in CI still? According to https://docs.github.com/en/actions/reference/environment-variables, all the env values that we use to determine colour support should be set.

[jorgebucaran]

All my colorette tests are passing after we merged your PR, so at least that worked.

I see you are using options.enabled, are you changing its value back after using it? It's basically a singleton.

See this test.

[mcollina]

why was this closed?

[kibertoad]

@mcollina I am still fighting with failing CI and don't want people flooded with notifications about interim commits.

[jorgebucaran]

@kibertoad For some reason NO_COLOR is in the environment, causing this:

let enabled =
    !isDisabled && (isForced || isWindows || isCompatibleTerminal || isCI)

...to always evaluate to false.

According to https://no-color.org:

NO_COLOR

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.

Manually setting colorette.options.enabled to true wouldn't work either, because colorized = colorizer(10) has already had the ANSI codes added via colorizer().

[jorgebucaran]

Maybe this is tap messing with NO_COLOR. I just replaced tap with tape, and all tests passed (on GitHub).

[kibertoad]

@jorgebucaran Great finding! I think there's an option for colours in tap, I'll try that.

[jorgebucaran]

d767067 probably won't work. tap sets NO_COLOR to 1 to comply with Chalk's implementation of NO_COLOR (which is inconsistent with the spec).

[kibertoad]

@jorgebucaran Seems to have worked, though, now the only reason why it is failing is code coverage that dropped for some reason.

[coveralls]

Pull Request Test Coverage Report for Build 1150442036

• 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 1116256302:	0.0%
Covered Lines:	305
Relevant Lines:	305

---

💛 - Coveralls

[kibertoad]

@mcollina @jsumners Ready for review!

[jsumners]

I'm concerned about the statement around some part of this new dependency being a singleton. How does that affect things?

[kibertoad]

@mcollina Adjusted PR to no longer modify state, now it works the same way it did with chalk, we just do readonly check of colorette colour support resolution results.

[kibertoad]

@jsumners See https://github.com/pinojs/pino-pretty/pull/218/checks?check_run_id=3353755751

[mcollina]

Code is good, we need to fix CI.

[jsumners]

@jsumners See #218 (checks)

Remove the glob from the npm script and add it to the .taprc:

files:
	- '**/*.test.js'

[kibertoad]

@jsumners That seems to completely break library imports for some reason.

update: ah, wait, looks like this glob then also includes the node_modules and tries to run tests from there

[kibertoad]

Removed Windows from CI for now, because it seems to be failing for reasons unrelated to colorette migration, but glob is fine now.

[kibertoad]

Any ideas why test: test/cli-rc.test.js cli throws on invalid default config file might be flaky on MacOS? Should I disable MacOS test execution?

[jsumners]

Any ideas why test: test/cli-rc.test.js cli throws on invalid default config file might be flaky on MacOS? Should I disable MacOS test execution?

Not a clue.

[jsumners]

Looks good to me.

[kibertoad]

@mcollina CI is green!

[mcollina]

@jsumners I think I should bump the major for this. WDYT?

[mcollina]

lgtm

[jsumners]

@jsumners I think I should bump the major for this. WDYT?

Agreed.