-
Notifications
You must be signed in to change notification settings - Fork 2
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
Frontend library bumps #95
Conversation
…create-react-app#11174 This allows us to run `npm audit --production` and see actual vulnerabilities we might care about, not thousands of irrelevant Regex DoS CVEs in transitive dependencies of our build tooling.
.nvmrc
Outdated
@@ -0,0 +1 @@ | |||
14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.nvmrc
files are handy for developers who use nvm to install multiple versions of node. You can just do nvm use
before running any commands.
Dockerfile
Outdated
@@ -1,4 +1,4 @@ | |||
FROM node:12-alpine AS builder | |||
FROM node:14-alpine AS builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node 12 is LTS until May 2022 but we might as well upgrade
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually ended up going to node 16, see comment below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fab, does this line need updating to 16 then? Also, I think we mention npm version in readme - can you update the text/link there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep have done in this commit. I think I forgot to push it last night 🤦
"@fortawesome/free-regular-svg-icons": "^5.13.0", | ||
"@fortawesome/free-solid-svg-icons": "^5.13.0", | ||
"@fortawesome/react-fontawesome": "^0.1.9", | ||
"@testing-library/jest-dom": "^4.2.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've not got any tests :) we can put these back when we add some
"@testing-library/jest-dom": "^4.2.4", | ||
"@testing-library/react": "^9.5.0", | ||
"@testing-library/user-event": "^7.2.1", | ||
"babel-polyfill": "^6.26.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I need to revisit this one, I thought Create React App would include the polyfills based on the browserlist
key but we might need to import it manually maybe (https://babeljs.io/docs/en/babel-polyfill.html).
We might not need polyfills at all, I wonder what the minimum Safari version is in use in the warehouse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm so a quick look at https://caniuse.com/es6 and Safari has not needed a polyfill since v10, release in Sep 20 2016. So I think we're probably fine? If not Create React App has their own polyfill we can use which doesn't tie us to Babel (I can dream that one day CRA will drop it haha)
"@testing-library/react": "^9.5.0", | ||
"@testing-library/user-event": "^7.2.1", | ||
"babel-polyfill": "^6.26.0", | ||
"cross-fetch": "^3.0.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure fetch
is supported in all the browsers we target and we're not doing any server-side rendering which is the other use case for these libraries
"babel-polyfill": "^6.26.0", | ||
"cross-fetch": "^3.0.4", | ||
"date-fns": "^2.13.0", | ||
"node-sass": "^4.14.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deprecated in favour of sass
, which as a bonus is pure javascript so no more horrible node-gyp
build failures after upgrades.
"cross-fetch": "^3.0.4", | ||
"date-fns": "^2.13.0", | ||
"node-sass": "^4.14.1", | ||
"prop-types": "^15.7.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only had PropTypes in one component and tbh I'd rather incrementally adopt Typescript than add more of them
"redux-thunk": "2.3.0" | ||
}, | ||
"devDependencies": { | ||
"react-scripts": "4.0.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With our build deps as devDependencies
we can run npm audit --production
to get the actual number of affected vulnerabilities not thousands of nonsense Regex DoS CVEs that only affect our build tools. This is official advice from Create React App too (facebook/create-react-app#11174)
"react-select": "^3.1.0", | ||
"redux": "^4.0.5", | ||
"redux-thunk": "^2.3.0", | ||
"typescript": "^3.8.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Typescript yet but we can add it back when we need it :)
@@ -1,5 +1,6 @@ | |||
@import 'include'; | |||
@use 'sass:math'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a deprecation warning, the division syntax is going away in future versions of sass
We still have some warnings in the build logs:
These are legendary: npm/cli#169. It's expected behaviour because
These two come from Create React App. The former should be fixed fairly quickly (facebook/create-react-app#11235), the latter is a long-standing issue that is difficult to solve without making Typescript usage harder (facebook/create-react-app#6834) |
Oh man, upgrading to node 16 removed those warnings but in come a whole bunch of deprecation ones instead!
They're all via Create React App and it seems their official policy is wait it out until their downstream dependencies upgrade: facebook/create-react-app#9431 (comment). I can totally understand but it really shows what a mess this ecosystem can be sometimes. I think we'll just have to live with it. What do you reckon @adamcunnington? |
"redux": "^4.0.5", | ||
"redux-thunk": "^2.3.0", | ||
"typescript": "^3.8.3" | ||
"@fortawesome/fontawesome-svg-core": "1.2.36", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pinned each version exactly. We've got a minimal set of direct dependencies but still better safe than investigating random errors because we pulled in a new version :)
The project is over a year old, a lifetime in the wonderful NPM ecosystem. I've gone through updating our dependencies and removing some that we don't use. I've done a quick smoke test manually on Firefox and Safari and things still seem fine.
I've added inline comments explaining the decisions I've taken as I went along