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

[vtadmin-web] Remove craco dependency #9591

Merged
merged 2 commits into from
Jan 31, 2022

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Jan 28, 2022

Signed-off-by: Sara Bee 855595+doeg@users.noreply.github.com

Description

This PR removes the craco dependency. Arguably the best kind of dependency "upgrade". 😎

I noticed this when running npm update:

🍕~/workspace/vitess/web/vtadmin $ npm update
npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR! 
npm ERR! While resolving: vtadmin@0.1.0
npm ERR! Found: react-scripts@5.0.0
npm ERR! node_modules/react-scripts
npm ERR!   react-scripts@"^5.0.0" from the root project
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer react-scripts@"^4.0.0" from @craco/craco@6.4.3
npm ERR! node_modules/@craco/craco
npm ERR!   @craco/craco@"^6.4.0" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR! 
npm ERR! See /Users/sarabee/.npm/eresolve-report.txt for a full report.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/sarabee/.npm/_logs/2022-01-28T20_05_21_769Z-debug.log

craco was added in #9215 in order to install tailwindcss@2.x. Thanks to @notfelineit by way of #9493, we're now using react-scripts@5.x which supports tailwindcss out of the box.

This means we no longer need craco, and indeed the tailwindcss@3.x docs no longer mention it.

We still need to upgrade to tailwindcss@3.x; as of this PR, we're still on npm:@tailwindcss/postcss7-compat@^2.2.17 (per the tailwindcss@v2 guide), which incidentally breaks npm outdated (TIL). I'll do the tailwindcss upgrade in a separate PR to keep things tidy.

I tested this change by running all of the scripts in package.json that previously relied on craco. I made sure to also test changes to the tailwind.config.js and base CSS files (to force a full tailwind rebuild). Everything runs + builds fine.

Related Issue(s)

Checklist

  • Should this PR be backported? No
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Not strictly necessary, but if you have an existing node_modules folder, I'd recommend doing npm install (or a full rm -rf node_modules && npm install, if you like). This is mostly relevant for developers rather than operators, as the vtadmin-web targets in the Makefile already do an npm install.

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Copy link
Contributor

@notfelineit notfelineit left a comment

Choose a reason for hiding this comment

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

Ah heck yeah! This is so nice!

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Copy link
Contributor Author

doeg commented Jan 31, 2022

@notfelineit I found one more craco-related thing we can delete (yay). Would you mind reviewing this one more time?

@doeg doeg merged commit d67588f into vitessio:main Jan 31, 2022
@doeg doeg deleted the sarabee-remove-craco branch January 31, 2022 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants