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

Merged
merged 1 commit into from
Oct 26, 2024
Merged

adopt picocolors #1769

merged 1 commit into from
Oct 26, 2024

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.

@mbostock
Copy link
Member Author

That’s a pre-existing problem with Cloud logs, right? E.g., this oss-analytics deploy. This makes it worse, but seems like something that shouldn’t block this PR.

Screenshot 2024-10-25 at 7 42 08 PM

@Fil
Copy link
Contributor

Fil commented Oct 26, 2024

OK then!

@mbostock mbostock merged commit ba7defb into main Oct 26, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/picocolors branch October 26, 2024 10:39
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