Skip to content

fix(deps): replace chalk with smaller and faster ansis #7058

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

serhalp
Copy link
Collaborator

@serhalp serhalp commented Feb 24, 2025

Summary

Replace chalk with ansis: https://github.com/webdiscus/ansis?tab=readme-ov-file#how-to-switch-to-ansis-1.

Honestly, I did this before realizing the savings are absolutely tiny (44 KB → 6 KB), but I fixed a few types along the way so we might as well ship it.

Copy link

github-actions bot commented Feb 24, 2025

📊 Benchmark results

Comparing with 33b60ba

  • Dependency count: 1,238 ⬆️ 0.08% increase vs. 33b60ba
  • Package size: 311 MB ⬆️ 0.01% increase vs. 33b60ba
  • Number of ts-expect-error directives: 415 ⬇️ 1.69% decrease vs. 33b60ba

// @ts-expect-error TS(7006) FIXME: Parameter 'optionA' implicitly has an 'any' type.
export const sortOptions = (optionA, optionB) => {
export const compareOptions = (optionA: Option, optionB: Option): number => {
const longOptionA = optionA.long ?? ''
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though we always define one, an option is allowed to have no long variant

Comment on lines +67 to 73
// FIXME(serhalp) This logic can't be right? Look at it... This must only happen to work.
if (BASE_FLAGS.has(longOptionA) || BASE_FLAGS.has(longOptionB)) {
return -1
}
Copy link
Collaborator Author

@serhalp serhalp Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewer, am I tripping? This should be something like this, right?

Suggested change
// FIXME(serhalp) This logic can't be right? Look at it... This must only happen to work.
if (BASE_FLAGS.has(longOptionA) || BASE_FLAGS.has(longOptionB)) {
return -1
}
if (BASE_FLAGS.has(longOptionA)) {
return 1 // A is a base flag, it should come last
} else if (BASE_FLAGS.has(longOptionB)) {
return -1 // B is a base flag, it should come last
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggestion looks to be correct. A negative value indicates that a should come before b, and a positive value indicates that a should come after b.

@serhalp serhalp force-pushed the fix/replace-chalk branch from 26aa3a2 to 4b74afe Compare April 10, 2025 19:04
@serhalp serhalp changed the title fix(deps): replace chalk with picocolors fix(deps): replace chalk with smaller ansis Apr 10, 2025
@serhalp serhalp force-pushed the fix/replace-chalk branch 3 times, most recently from d3883ca to 664df79 Compare April 10, 2025 20:15
@serhalp serhalp changed the title fix(deps): replace chalk with smaller ansis fix(deps): replace chalk with smaller and faster ansis Apr 10, 2025
@serhalp serhalp force-pushed the fix/replace-chalk branch from 664df79 to 13eeded Compare April 10, 2025 20:16
Comment on lines +219 to +221
// This is not possible, but since the fixed set of choices contains one dynamically interpolated string,
// we can't tell TS that these are exhaustive values
return logAndThrowError(new Error('Invalid link type selected'))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see lines 31-32

@@ -16,6 +16,10 @@ export const callCli = async function (args, execOptions = {}, parseJson = false
const { stdout } = await execa.node(cliPath, args, {
timeout: CLI_TIMEOUT,
nodeOptions: [],
env: {
// TODO(serhalp) Why not exercise colorization in integration tests? Remove and update snapshots?
NO_COLOR: '1',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chalk was doing this magically in our tests, so this is just preserving that behaviour for now

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhalp

some tests failed because expected /^VERSION/ but received the string \u001b[1mVERSION\u001b[22m\n containing ANSI codes.

Ansis automatically detects CI environments as supported color.

To force disable color should be created separate file with the NO_COLOR variable, e.g. no-color.js:

// set env variable to force disable color
process.env.NO_COLOR = '1';

This file must be imported in the test file before other files used ansis:

import { expect, describe, test } from 'vitest';

// import env variables to force disable color
import './no-color.js';

// ... import others

Alternatively, you can use the ansis.strip() function to clean a string from ANSI codes:

e2e/install.e2e.ts:179

import { expect, describe, test } from 'vitest';
import ansis from 'ansis';
// ...

expect(ansis.strip(stdout.trim()), `Help command does not start with 'VERSION':\n\n${ansis.strip(stdout)}`).toMatch(/^VERSION/)

See please the no-color.test.js

@serhalp serhalp force-pushed the fix/replace-chalk branch 2 times, most recently from 98e3f92 to 3513229 Compare April 10, 2025 21:06
@serhalp serhalp force-pushed the fix/replace-chalk branch from 3513229 to f39e193 Compare April 11, 2025 14:56
Oops and I got carried away and fixed a lot of types along the way.
@serhalp serhalp force-pushed the fix/replace-chalk branch from f39e193 to 46ca5d8 Compare April 11, 2025 21:25
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.

3 participants