-
Notifications
You must be signed in to change notification settings - Fork 469
chore(deps): replace chalk with picocolors #1341
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
Conversation
I saw that @testing-library/jest-dom is also using chalk but only in a test. However, it is listed as dependency. Maybe we can move it to devDependencies and also replace it?
It's listed in devDependencies as far as I can tell.
CI errors may be happening without this change and need an update to our build pipeline. Do you think you can fix it?
@eps1lon I looked into it and this is a deadlock. I tried several combinations of pinned dependencies and got the tests to work eventually. But now validate fails.
I really wonder why nobody ever bothered to commit a lock-file. Thats exactly what it is for :D.
The reason this is all not working is, is because some babel dependencies that are brought in require at least babel 7.22. However, that version calls browserlist with a different api (and therefore fails). Installing older versions of those packages gets rid of that error but now the validate fails with (to me unknown) reason because it complains that a function isnt found (which is clearly there).
So I have to give up for now. What other possibilities do we have?
Does anyway of you have a working setup? If so, please dump all the exact versions of your dependencies by (deleting any shrinkwrap and lock file and then) running
npm shrinkwrap
Give me that file :D
// EDIT: CI: it looks like its a dependency and not in dev: https://github.com/testing-library/jest-dom/blob/main/package.json
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1bb91ea:
|
OMG I DID IT!!!!
I had to commit the package-lock.json to convince the CI to install the correct versions.
So not sure what to do with this.
It would be really beneficial to get rid of old supported browsers so we can update browserslist so we can update babel so we can get rid of all that overwrites and really unstable depenceny situation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request @Fuzzyma !
I did leave some comments, feel free to ask for assistance if needed.
@timdeschryver I reverted the commit of the package-lock and the ci is red again. So would love some help here. Clearly the CI does something with the lock-file :D
Also moved typescript back to devDependencies where it rightfully belongs
It seems like that there was a push to introduce a lockfile in 2020 already but Kent wanted the approval from one other maintainer (#624 (comment)). Maybe that could be you? :D
Kent also seems to use lockfiles now:
https://github.com/kentcdodds/kentcdodds.com
Kent also seems to use lockfiles now:
https://github.com/kentcdodds/kentcdodds.com
@Fuzzyma Kent's not opposed to using lock files in projects/apps, but the way I understood it he is opposed using them in libraries/packages
I will take a look at this in the next days (I might push directly into this branch).
AFAIK @MichaelDeBoey is right about the reasons of why not to include a lock file, additionally it also allows people to use their package manager of choice. We might revisit this later, but I prefer not to do this within this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinning @babel/helper-compilation-targets seems to do the trick.
I used the last version of a successful build.
This has three approvals. Could we merge it?
I would also recommend merging this related PR: testing-library/jest-dom#659 (chalk 3 is used there, so we're currently pulling in two versions of chalk. I.e. one from here and one from there)
Thanks @benmccann for the ping.
I've just merged testing-library/jest-dom#659, let's see we're not seeing any regressions there and I'll merge this one in the upcoming week :)
benmccann
commented
Jul 26, 2025
Thank you!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the trouble you had! I'm fine including lockfiles now. I wood have been fine inlining the style for the one use here as well, though this needs to run in the browser as well, so we should probably keep the dependency.
At some point we should probably modernize the CI and other tools in this project as well.
Thank you!
Uh oh!
There was an error while loading. Please reload this page.
Replaced
chalkwithpicocolorsas discussed in #1340.I ended up using picocolors to support the NO_COLOR env (https://no-color.org/)
Checklist:
I wasnt able to run the tests. It errored with
TypeError: _browserslist.findConfigFile is not a function.I dont want to call this mergable unless I can see the green tests.
// EDIT: The CI seems to fail with the same error
// OFFTOPIC: I saw that
@testing-library/jest-domis also using chalk but only in a test. However, it is listed as dependency. Maybe we can move it to devDependencies and also replace it?