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

adopt picocolors #1769

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

adopt picocolors #1769

wants to merge 1 commit into from

Conversation

mbostock
Copy link
Member

Replaces our hand-rolled version with the popular picocolors, offering support for NO_COLOR and relying on picocolor’s own color support detection. As an alternative, we could also use yoctocolors which already uses hasColors… but I figured picocolors is nice because it’s already a dependency of clack.

Related #1764.

@mbostock mbostock requested a review from Fil October 19, 2024 23:16
@mbostock mbostock enabled auto-merge (squash) October 19, 2024 23:16
@Fil
Copy link
Contributor

Fil commented Oct 21, 2024

It works in local tests (with say, yarn docs:build | tee build.log), but doesn't work at all on cloud: my logs are full of color codes.
https://observablehq.com/projects/@observablehq/pangea/deploys/e9b695e6f5001670

logs

When I look at the source code, the logic is defaulting to using colors when !!env.CI. In other words, it is expected that CI always supports ANSI colors.
https://github.com/alexeyraspopov/picocolors/blob/7249f8c5d4825550f70bc1ea98652639933d3bbd/picocolors.js#L4

I think it's a fair assumption, and that we should merge this in nonetheless, when we're ready.

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

Successfully merging this pull request may close these issues.

2 participants