Skip to content
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

chore(deps): replace chalk with picocolors #1341

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Fuzzyma
Copy link

@Fuzzyma Fuzzyma commented Nov 19, 2024

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?

@eps1lon
Copy link
Member

eps1lon commented Nov 19, 2024

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?

@Fuzzyma
Copy link
Author

Fuzzyma commented Nov 19, 2024

@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

Copy link

codesandbox-ci bot commented Nov 20, 2024

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:

Sandbox Source
react-testing-library-examples Configuration

@Fuzzyma
Copy link
Author

Fuzzyma commented Nov 20, 2024

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

Copy link
Member

@timdeschryver timdeschryver left a 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.

.gitignore Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@Fuzzyma
Copy link
Author

Fuzzyma commented Nov 21, 2024

@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
Copy link
Author

Fuzzyma commented Nov 21, 2024

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

@MichaelDeBoey
Copy link
Member

MichaelDeBoey commented Nov 21, 2024

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

@timdeschryver
Copy link
Member

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.

Copy link
Member

@timdeschryver timdeschryver left a 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.

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.

4 participants