-
Notifications
You must be signed in to change notification settings - Fork 5
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: remove EOL node versions from CI #247
Conversation
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.
Since this PR is dedicated to changing node versions, I'd suggest adding a .nvmrc
file to make the environment setup easier for the developers. ( Reference )
@@ -8,8 +8,6 @@ | |||
"bootstrap": "^4.0.0", | |||
"d3-selection": "^1.3.2", | |||
"d3-zoom": "^1.7.3", | |||
"eslint-config-airbnb": "^16.1.0", | |||
"eslint-plugin-react": "^7.5.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.
I agree on removing these since we don't use them directly on this project right now.
But I think it's important to have a linting system available for all our projects. For now, I'll just reinforce the issue that is already open on this matter
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.
react-scripts has eslint as a dependency, so it is already installed, we just need a configuration, which can be done with a package.json key eslintConfig
.
The best option would be to use the react-scripts default configuration but it may be extended for custom rules.
* chore: remove eol node versions from CI
Acceptance Criteria
Why enable openssl legacy provider
In Node.js v17 a security issue from the SSL provider was fixed, this is a breaking change.
To use node 17 or later we have to either enable the openssl legacy provider flag or upgrade the dependencies to not use the affected libraries.
The upgrade can be done via
npm audit fix --force
but in our case it also required the react-scripts to be upgraded 2 major versions, this new react-scripts uses webpack 5 for build.Webpack 5 does not automatically include polyfills for node libraries, but we cannot manually add them since react-scripts does not allow custom webpack configs.
The issue can be solved in the wallet-lib by creating a browser compatible build but this requires an upgrade to the wallet-lib version which cannot be done currently since the explorer uses many deprecated apis on the wallet-lib.
The logic solution is to enable the legacy provider, understanding the security issues of openssl and create a project to refactor the explorer for the new wallet-lib version while also making the wallet-lib browser compatible.
Why remove eslint libs from package.json
We do not use them directly and the version we had fixed required an eslint version with a conflict with the react-scripts required eslint version.
So removing them is the best option.
Security Checklist