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] Update react-scripts, postcss #9493

Merged
merged 6 commits into from
Jan 11, 2022
Merged

Conversation

notfelineit
Copy link
Contributor

@notfelineit notfelineit commented Jan 10, 2022

Description

This PR updates some vulnerable npm packages, namely:

React Scripts
react-script 5.0.0 has added support for tailwindcss. Followed the steps outlined here: facebook/create-react-app#11717 (comment) to make current version compatible. That includes manually installing necessary postcss plugins.

There are also postcss config updates as suggested here: facebook/create-react-app#11777

Testing

Manual testing was done here, made sure the vtadmin portal rendered "as normal" - primarily the debug page with all the tailwind styles 😃

Related Issue(s)

N/A, run npm audit fix

Checklist

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

Deployment Notes

None

Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
@notfelineit notfelineit added Component: VTAdmin VTadmin interface dependencies Pull requests that update a dependency file Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Jan 10, 2022
Signed-off-by: notfelineit <notfelineit@gmail.com>
Signed-off-by: notfelineit <notfelineit@gmail.com>
@@ -1,3 +1,4 @@
/* eslint-disable jest/no-conditional-expect */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of updating the test, I decided to use eslint-disable here.

@@ -86,7 +86,7 @@ describe('Tablets', () => {
test.each(tests.map(Object.values))(
'%s',
(name: string, filter: string, tablets: pb.Tablet[], expected: { [k: string]: unknown }[]) => {
const result = formatRows(tablets, filter);
const result = formatRows(tablets, [], filter);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a type issue - formatRows expects 3 params.

@@ -25,17 +25,20 @@
"highcharts-react-official": "^3.0.0",
"history": "^5.0.0",
"lodash-es": "^4.17.20",
"postcss": "^7.0.39",
"postcss": "^8.4.5",
"postcss-flexbugs-fixes": "^5.0.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These have to be installed manually for postcss ^8 I think.

@setassociative
Copy link
Contributor

Unless the upgrade to react-scripts brought more restrictive type checking in I would have preferred to see the test changes and lint updates to be spun into a separate review. My intuition is that these started firing with the eslint upgrade though and so that's why they got pulled in here?

In any event these all seem safe enough to me. I'm curious about the need to export tokenize.Token though and what underlying type checker / build change caused that.

Light suggestion would be to include a testing section in the PR template -- I'm assuming that you did a round of app builds and did some coarse grain validation passes (app loads, CSS loads as expectd, etc).

LGTM 🚢

@frouioui
Copy link
Member

@deepthi notified me of a failure in the Upgrade Downgrade Testing Query Serving workflow that can be observed here. I created a pull request to fix the issue (#9498), the description of the issue is written in the pull request. I recommend rebasing on top of #9498, or waiting for #9498 to be merged onto main, then your pull request' checks will turn green 🎉

@notfelineit
Copy link
Contributor Author

@setassociative Indeed, updating react-scripts caused these untouched files to fail eslint CI, so I had to fix them 😄 The same thing happened with tokenize.Token - it got flagged as a type error bc it was not being exported!

@notfelineit
Copy link
Contributor Author

notfelineit commented Jan 11, 2022

Thanks @frouioui !!

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Approving based on the review from @setassociative

@deepthi deepthi merged commit c2ba091 into main Jan 11, 2022
@deepthi deepthi deleted the frances/update-react-scripts branch January 11, 2022 19:24
@doeg doeg mentioned this pull request Feb 7, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTAdmin VTadmin interface dependencies Pull requests that update a dependency file Type: Enhancement Logical improvement (somewhere between a bug and feature)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants