Replace kleur with picocolors
#14598
Conversation
🦋 Changeset detectedLatest commit: 0f938c1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
pkg: integration
CodSpeed Performance ReportMerging #14598 will not alter performanceComparing Summary
Footnotes |
| import { exec } from './exec.js'; | ||
|
|
||
| const require = createRequire(import.meta.url); | ||
| const { bold, cyan, dim, magenta } = colors; |
There was a problem hiding this comment.
In most modules, I refactored to use colors.method() directly, but in a couple like this one where there’s heavy use of the APIs, I kept the destructured approach to minimize code changes and keep usage a bit cleaner.
| return mv.annotation | ||
| ? `${style(String(mv.value))} ${dim(mv.annotation)}` | ||
| : style(String(mv.value)); |
There was a problem hiding this comment.
I suspect this isn’t technically required, but picocolors types don’t allow boolean arguments like kleur did, and the usage here sometimes includes them. Coercing with String() every time should be fine in this case given the astro preferences CLI is rarely run and not performance sensitive.
| // Disable colors when running tests | ||
| kleur.$.enabled = false; |
There was a problem hiding this comment.
I wasn’t 100% sure of the intention here, but tests pass without this and picocolors does not have an equivalent API, so I removed it.
| import colors from 'picocolors'; | ||
| import type { TextStyler } from '../definitions.js'; | ||
|
|
||
| export function createTextStyler(): TextStyler { |
There was a problem hiding this comment.
Can you rename it to createPicolorsTextStyler() (and the filename)? Like here it's useful to have it in the name, it's not an implementation detail
There was a problem hiding this comment.
Oh, interesting!
Why do we have a generic TextStyler type then? Feels like if it’s not an implementation detail, wouldn’t it use a type of its own? And does that mean we’d have multiple stylers?
It just felt weird that to switch out a package, I also had to rename this, but maybe that’s intentional.
2 participants
Changes
picocolorsinstead ofkleurfor terminal text stylingpicocolorsis slightly smaller and faster, but more importantly, after perf: switch to clack for CLI prompts #14589 none of our other dependencies will be usingkleur, so switching like this would mean we align with what the rest of our dependencies use for colours, allowing us to drop a package from our dependency treekleur vs picocolors benchmarks
Source: https://github.com/alexeyraspopov/picocolors#benchmarks
Testing
Did some quick local CLI commands to test it out. Existing CI should pass.
Docs
n/a implementation detail