dom-testing-library icon indicating copy to clipboard operation
dom-testing-library copied to clipboard

chore(deps): replace chalk with picocolors

Open Fuzzyma opened this issue 1 year ago • 9 comments

Replaced chalk with picocolors as discussed in #1340. I ended up using picocolors to support the NO_COLOR env (https://no-color.org/)

Checklist:

  • [ ] Documentation added to the docs site N/A
  • [ ] Tests
  • [ ] TypeScript definitions updated N/A
  • [ ] Ready to be merged

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-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?

Fuzzyma avatar Nov 19 '24 20:11 Fuzzyma

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 avatar Nov 19 '24 21:11 eps1lon

@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

Fuzzyma avatar Nov 19 '24 22:11 Fuzzyma

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 1bb91ead71ccf539d5081eb88ba68a7dc5f048a9:

Sandbox Source
react-testing-library-examples Configuration

codesandbox-ci[bot] avatar Nov 20 '24 23:11 codesandbox-ci[bot]

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

Fuzzyma avatar Nov 20 '24 23:11 Fuzzyma

@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

Fuzzyma avatar Nov 21 '24 19:11 Fuzzyma

It seems like that there was a push to introduce a lockfile in 2020 already but Kent wanted the approval from one other maintainer (https://github.com/testing-library/dom-testing-library/pull/624#issuecomment-643260300). Maybe that could be you? :D

Kent also seems to use lockfiles now: https://github.com/kentcdodds/kentcdodds.com

Fuzzyma avatar Nov 21 '24 20:11 Fuzzyma

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

MichaelDeBoey avatar Nov 21 '24 20:11 MichaelDeBoey

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.

timdeschryver avatar Nov 22 '24 19:11 timdeschryver

@eps1lon does this seem good to you? As mentioned in the comments, @babel/helper-compilation-targets was pinned to make the build happy without upgrading deps/changing the browserlist support (as was done in #1346)

timdeschryver avatar Jan 24 '25 13:01 timdeschryver

This has three approvals. Could we merge it?

I would also recommend merging this related PR: https://github.com/testing-library/jest-dom/pull/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)

benmccann avatar Jul 26 '25 18:07 benmccann

Thanks @benmccann for the ping. I've just merged https://github.com/testing-library/jest-dom/pull/659, let's see we're not seeing any regressions there and I'll merge this one in the upcoming week :)

MatanBobi avatar Jul 26 '25 19:07 MatanBobi

Thank you!!

benmccann avatar Jul 26 '25 19:07 benmccann

Thank you!

kentcdodds avatar Jul 27 '25 13:07 kentcdodds