Skip to content

chore(deps): replace chalk with native util.styleText#5949

Open
roli-lpci wants to merge 2 commits intomozilla:masterfrom
roli-lpci:chore/replace-chalk-with-native-styletext
Open

chore(deps): replace chalk with native util.styleText#5949
roli-lpci wants to merge 2 commits intomozilla:masterfrom
roli-lpci:chore/replace-chalk-with-native-styletext

Conversation

@roli-lpci
Copy link
Copy Markdown

Summary

  • Replaces chalk dependency with Node.js native util.styleText API
  • Bumps minimum Node.js version from >=18.0.0 to >=20.12.0 (required for util.styleText)
  • All color methods use String() coercion for safe non-string handling

Changes

  • src/linter.jschalk.Instance({ enabled }) replaced with plain object of color functions that delegate to styleText() or pass through via String in boring mode
  • tests/integration/run-as-production-env.jschalk.red()/chalk.green() replaced with styleText('red', ...)/styleText('green', ...)
  • package.json — Removed chalk, bumped engines.node and browserslist to >=20.12.0

Testing

  • 66/66 test suites pass, 1700 tests
  • ESLint: 0 errors
  • Webpack production build: clean
  • No test changes needed (existing stubs work with new object shape)

Addresses #5932

Replace chalk dependency with Node.js native `util.styleText` API:
- chalk.red/yellow/blue → styleText('color', String(text))
- chalk.Instance({ enabled: false }) → String identity function
- Bump engines.node to >=20.12.0 (required for util.styleText)

Addresses mozilla#5932

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

Bumping the Node version would require a major version bump, which is too big of an ask for this refactor.

Please maintain support for older versions. I consider it to be an acceptable degradation of functionality by falling back to just printing the message as is without coloring (for simplicity), or including a simple, minimal set of escape sequences if you'd like to maintain the coloring on older Node.

Considering that Node 18 is already EOL for almost a year at this point, with Node 20 to follow soon (in less than 2 months), I think that we don't have to worry about old Node.

Therefore I'd be inclined to go with the simple approach: no coloring as a fallback.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 26, 2026

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 98.76%. Comparing base (d4e283e) to head (ba0ae38).

Files with missing lines Patch % Lines
src/linter.js 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5949      +/-   ##
==========================================
- Coverage   98.79%   98.76%   -0.04%     
==========================================
  Files          51       51              
  Lines        2905     2912       +7     
  Branches      895      898       +3     
==========================================
+ Hits         2870     2876       +6     
- Misses         35       36       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Use Node.js native styleText (19.10+) instead of chalk package for terminal color output. Removes unnecessary dependency while maintaining color output functionality.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

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

How did you generate the package-lock.json file? I see that it includes several changed, and it is unclear why these changes were made.

I checked the package-lock file to see if the removed dependency would disappear. While the entry is deleted from the dependencies, it is still listed. I checked mentions of chalk" in the lockfile and it seems that there is one dependent (eslint) that itself is a peer dependency, and others but those are just dev dependencies.

As the code changes itself seem to be AI-generated, I'd like to know whether the lock file was also AI-generated, or created by standard tooling. And if so, what versions did you use?

@roli-lpci
Copy link
Copy Markdown
Author

Thanks for the thorough review!

Re: Node version support — The fallback is already in place in the current code. Both src/linter.js and tests/integration/run-as-production-env.js check typeof util.styleText === 'function' before using it, and fall back to returning plain text without coloring on older Node versions (< 20.12). The engines field in package.json remains >=18.0.0 — no version bump.

Re: package-lock.json — Generated by standard npm install after removing chalk from package.json. The extra changes you see (mostly "peer": true additions on packages like eslint, webpack, prettier, jest, etc.) are npm re-resolving the dependency graph: packages that chalk previously pulled into the resolved tree now only appear as peer dependencies since their direct dependency chain through chalk is gone. This is normal npm lockfile behavior when removing a dependency.

Happy to squash or make any other adjustments.

@Rob--W
Copy link
Copy Markdown
Member

Rob--W commented Feb 27, 2026

I also asked for the versions of the tooling that you used, for reproducibility. In this case the Node and npm version.

Are you an autonomous AI or is there a human in the loop?

@roli-lpci
Copy link
Copy Markdown
Author

Node v25.2.1, npm 11.6.2.

I am what many would call a nontechnical developer with a pipeline that is highly steered, monitored, submitting PRs while I learn proper orchestration, validation, testing, etc. If you would not like any more submissions from me, let me know and I can abstain going forward. Thanks!

@roli-lpci
Copy link
Copy Markdown
Author

Hi Rob, my initial response didn't answer your question directly, so let me fix that.

Yes, the code changes are AI-assisted. I use an AI coding assistant as part of my development workflow. I direct the work, review the output, test locally, and decide what to submit. I think maintainers should know what they're reviewing so they can calibrate their scrutiny accordingly.

One finding from this migration that might be useful to the project regardless of whether this PR lands: styleText throws ERR_INVALID_ARG_TYPE on non-string arguments, unlike chalk which silently coerces them. The risk spot in this codebase is handleError in linter.js, where err.message || err can pass a raw Error object when message is empty. That's why every styleText call in this PR wraps with String(). Any future migration away from chalk would need to audit call sites for non-string arguments.

If this PR isn't a fit given the AI-assisted workflow, or you'd rather I not submit to this repo going forward, no hard feelings at all.

Rolando Bosch

@Rob--W
Copy link
Copy Markdown
Member

Rob--W commented Feb 28, 2026

Hi Roland, thanks for the human reply and your additional finding. I appreciate it.

I'm not opposed to AI-aided contributions when disclosed properly. I have mentored many contributions over the years, where I take time to explain and guide contributions. I noticed a recent trend where my feedback results in code changes or replies that are increasingly larger pieces of text, which takes more effort and time to review on my part.

The PR you generated looks reasonable to me, as it is an improvement that drops an unnecessary dependency. I do wonder whether the shape of the patch is the desired outcome, because the current fix is a drop-in replacement for the original chalk API, and even retains the naming. Other options are to update the callers and/or put the code in a separate module to allow reuse by the current callers of chalk. That won't affect functionality, but may improve maintainability. I planned to raise this with my colleagues to get another perspective on whether to take the PR as is or whether to request these changes.

Back to reflect on AI:
All of my feedback and requests would have been the same for a fully human contributor, in this case. A LLM can produce convincingly looking changes that look reasonable at the first glance but are actually incorrect. That is why I want to generate the lockfile myself before considering to merge.

@Rob--W
Copy link
Copy Markdown
Member

Rob--W commented Mar 12, 2026

@roli-lpci In the meantime we have bumped the minimum supported Node version to 20, just two days ago. We are willing to bump the minimum version to 20.12.0, to remove the need for having a fallback in the code.

Could you please rebase your PR, and change engines.node and browserslist.node to require >=20.12.0? The initial commit here (without ba0ae38) is essentially in a good shape.

This PR also migrates the use of chalk in tests/integration/run-as-production-env.js. Instead of migrating, let's drop the use of chalk / styleText there altogether. This file is only used in CI and development, and the colors do not add too much. Having fewer dependencies is preferred.

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