Skip to content
This repository has been archived by the owner on Apr 11, 2023. It is now read-only.

KIALI-2077 React related libraries upgrades #864

Merged
merged 14 commits into from
Jan 10, 2019
Merged

Conversation

mtho11
Copy link
Contributor

@mtho11 mtho11 commented Dec 4, 2018

** Describe the change **
Upgrade all of the React related libraries and Patternfly. And this time the redux upgrades because they are minor since we did a major upgrade last sprint for them.

** Issue reference **
https://issues.jboss.org/browse/KIALI-2077

** Backwards compatible? **
Yes

[ ] Is your pull-request introducing changes in behaviour?
No changes in behaviour

@mtho11 mtho11 added do not merge A PR is not ready to merge library change A PR that updates a frontend library labels Dec 4, 2018
@abonas
Copy link
Contributor

abonas commented Dec 4, 2018

is this issue linked correctly to JIRA?

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 5, 2018

Looks like our polyfill for url-search-params has been deprecated in favor of @ungap/url-search-params
https://github.com/WebReflection/url-search-params

@mtho11 mtho11 changed the title React related libraries upgrades KIALI-2077: React related libraries upgrades Dec 5, 2018
@mtho11
Copy link
Contributor Author

mtho11 commented Dec 5, 2018

@abonas no it was not correctly linked yet, I had to create a subtask Jira for it after I started it. Resolved now.

@mtho11 mtho11 changed the title KIALI-2077: React related libraries upgrades KIALI-2077 React related libraries upgrades Dec 5, 2018
@mtho11
Copy link
Contributor Author

mtho11 commented Dec 5, 2018

Also relevant to standards: https://developers.google.com/web/updates/2016/01/urlsearchparams

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 6, 2018

So now we have all these Snyk tests:
image
Probably others have not seen it because they don't have it installed

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 6, 2018

@theute what is our policy with Snyk in regards to being Done.

@theute
Copy link
Contributor

theute commented Dec 6, 2018

I am not sure about the question, if the check is in place, I guess it should pass ;) (Being Captain Obvious here).

Now what's odd I guess is that your PR may fail just because a new vulnerability has been found but nothing related was changed, correct ?

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 6, 2018

@theute So I'm currently trying to remediate any vulnerabilities but have yet to see if this is possible.
We'll see how this goes. I guess it's just a new part of Dev process that we haven't fully fleshed out yet. -- Snyk Remediation
We don't yet have any rules on what level severity is blocking and what is ok?

@cfcosta
Copy link
Contributor

cfcosta commented Dec 11, 2018

@mtho11 I'd say, for snyk, anything over medium should be considered a blocker, and even medium in case it contains tthe possibility for RCE.

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 11, 2018

This is a weird error that stops the build:

@kiali/kiali-ui@0.12.0 build-css /home/travis/build/kiali/kiali-ui
node-sass-chokidar src/ --output-style compressed --include-path $npm_package_sassIncludes_src --include-path $npm_package_sassIncludes_patternfly --include-path $npm_package_sassIncludes_patternflyReact --include-path $npm_package_sassIncludes_bootstrap --include-path $npm_package_sassIncludes_fontAwesome -o src/
Wrote 10 CSS files to /home/travis/build/kiali/kiali-ui/src/
? We're unable to detect target browsers.
Would you like to add the defaults to your package.json? (Y/n)

This doesn't happen locally. Unable to detect target browsers.

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 13, 2018

So as part of the react-scripts-ts upgrade (2 -> 4) it looks like autoprefixer pulled in browserslist and so now we need to have a section in the package.json to specify what browsers we are supporting so that autoprefixer knows.

Valid Queries:
https://www.npmjs.com/package/browserslist#queries
I choose:

"browserslist": [
    ">3%",
    "last 2 versions",
    "not dead",
    "not ie <= 11"
  ]

To see a list of supported browsers type: npx browserslist
And here is the list of browsers it generated:

and_chr 70
and_ff 63
and_qq 1.2
and_uc 11.8
android 67
android 4.4.3-4.4.4
baidu 7.12
chrome 70
chrome 69
edge 18
edge 17
firefox 63
firefox 62
ie_mob 11
ios_saf 12-12.1
ios_saf 11.3-11.4
op_mini all
op_mob 46
opera 57
opera 56
safari 12
safari 11.1
samsung 7.2
samsung 6.2

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 13, 2018

@abonas does the browser list above look about right to you?

@mtho11 mtho11 force-pushed the react-upgrades branch 3 times, most recently from 6018ebf to 5bb0ba5 Compare December 17, 2018 11:45
@mtho11 mtho11 removed the do not merge A PR is not ready to merge label Dec 17, 2018
@mtho11
Copy link
Contributor Author

mtho11 commented Dec 17, 2018

This last commit fixes this bug:

Uncaught TypeError: url__WEBPACK_IMPORTED_MODULE_5__.URLSearchParams is not a constructor
    at Metrics.push../src/components/Metrics/Metrics.tsx.Metrics.render (Metrics.tsx:222)
    at finishClassComponent (

Which started occuring on certain screens that use the url library.

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 20, 2018

Thanks @jotak @cfcosta @hhovsepy and @mattmahoneyrh for your input. We are closing in...

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 20, 2018

Also, the reason for moving from react-script-ts 4.x instead of 3.1 is that snyk found a couple vulnerabilities in 3.x:

✗ Low severity vulnerability found in webpack-dev-server
  Description: Information Exposure
  Info: https://snyk.io/vuln/SNYK-JS-WEBPACKDEVSERVER-72405
  Introduced through: react-scripts-ts@3.1.0
  From: react-scripts-ts@3.1.0 > webpack-dev-server@2.11.3
  Remediation:
    Upgrade direct dependency react-scripts-ts@3.1.0 to react-scripts-ts@4.0.1 (triggers upgrades to react-scripts-ts@4.0.1 > webpack-dev-server@3.1.9)

✗ Low severity vulnerability found in merge
  Description: Prototype  Pollution
  Info: https://snyk.io/vuln/SNYK-JS-MERGE-72553
  Introduced through: react-scripts-ts@3.1.0
  From: react-scripts-ts@3.1.0 > jest@20.0.4 > jest-cli@20.0.4 > jest-haste-map@20.0.5 > sane@1.6.0 > exec-sh@0.2.2 > merge@1.2.0
  From: react-scripts-ts@3.1.0 > jest@20.0.4 > jest-cli@20.0.4 > jest-runtime@20.0.4 > jest-haste-map@20.0.5 > sane@1.6.0 > exec-sh@0.2.2 > merge@1.2.0
  Remediation:
    Your dependencies are out of date, otherwise you would be using a newer version of merge.
    Try deleting node_modules, reinstalling and running `snyk test` again. If the problem persists, one of your dependencies may be bundling outdated modules.

✗ Low severity vulnerability found in braces
  Description: Regular Expression Denial of Service (ReDoS)
  Info: https://snyk.io/vuln/npm:braces:20180219
  Introduced through: react-scripts-ts@3.1.0
  From: react-scripts-ts@3.1.0 > webpack-dev-server@2.11.3 > http-proxy-middleware@0.17.4 > micromatch@2.3.11 > braces@1.8.5
  From: react-scripts-ts@3.1.0 > fork-ts-checker-webpack-plugin@0.2.10 > chokidar@1.7.0 > anymatch@1.3.2 > micromatch@2.3.11 > braces@1.8.5
  From: react-scripts-ts@3.1.0 > jest@20.0.4 > jest-cli@20.0.4 > jest-jasmine2@20.0.4 > jest-message-util@20.0.3 > micromatch@2.3.11 > braces@1.8.5
  and 16 more...
  Remediation:
    Upgrade direct dependency react-scripts-ts@3.1.0 to react-scripts-ts@4.0.1 (triggers upgrades to react-scripts-ts@4.0.1 > webpack-dev-server@3.1.9 > http-proxy-middleware@0.18.0 > micromatch@3.1.9 > braces@2.3.1)
    Upgrade direct dependency react-scripts-ts@3.1.0 to react-scripts-ts@4.0.1 (triggers upgrades to react-scripts-ts@4.0.1)

@cfcosta
Copy link
Contributor

cfcosta commented Dec 20, 2018

@mtho11 I’d say go with 3, even with the vulnerabilities, they all have low risk and do not seem exploitable.

@mtho11
Copy link
Contributor Author

mtho11 commented Dec 20, 2018

@cfcosta I'm actually trying to get rid of react-scripts-ts and use the standard react-scripts now that the 2.x version supports typescript natively. Like this article talks about: Migrating from create-react-app-typescript to Create React App

@mtho11
Copy link
Contributor Author

mtho11 commented Jan 8, 2019

@cfcosta our previously low severity vulnerability in react-scripts-ts version 3 is now upgraded to high severity. :(

✗ High severity vulnerability found in webpack-dev-server
  Description: Information Exposure
  Info: https://snyk.io/vuln/SNYK-JS-WEBPACKDEVSERVER-72405
  Introduced through: react-scripts-ts@3.1.0
  From: react-scripts-ts@3.1.0 > webpack-dev-server@2.11.3

@mtho11
Copy link
Contributor Author

mtho11 commented Jan 8, 2019

The application works fine and js console is clean. @mattmahoneyrh was kind enough to deploy this PR to: https://kiali-istio-system.openshift2.jonqe.lab.eng.bos.redhat.com/console/overview?duration=60&pi=15000

@mtho11
Copy link
Contributor Author

mtho11 commented Jan 9, 2019

To summarize: Right now I'm in the unfortunate position of being trapped between version 3 of react-scripts-ts which works but has a high vulnerability and version 4 that doesn't render our app. For reference, we were on version 2.x of react-script-ts.

Caveat: The tests for version 3 are failing but are probably fixable. However, if version 3 is not a viable option I will not spend any time on that solution.

@mtho11
Copy link
Contributor Author

mtho11 commented Jan 9, 2019

So I'm just going to leave the version of react-scripts-ts at the 2.x (what it was before the upgrade). This library is just a development infrastructure library. All the react related libs are upgraded to 16.6.3. We decided to stay one version back on the react release because other libraries like @types/* need time to upgrade to the newer react releases.

So @cfcosta this is done and ready for retest

@cfcosta
Copy link
Contributor

cfcosta commented Jan 9, 2019

@mtho11 ok, perfect, will take a look.

@cfcosta
Copy link
Contributor

cfcosta commented Jan 9, 2019

@mtho11 Did a ton of tests, everything seems ok. Will approve, should we merge it now?

@mtho11
Copy link
Contributor Author

mtho11 commented Jan 9, 2019

Since I removed the trouble-maker library. I think we are good now.

@mtho11 mtho11 merged commit 595ab2d into kiali:master Jan 10, 2019
@mtho11 mtho11 deleted the react-upgrades branch January 10, 2019 15:05
@mtho11 mtho11 removed the do not merge A PR is not ready to merge label Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
library change A PR that updates a frontend library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants