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

chore: remove EOL node versions from CI #247

Merged
merged 3 commits into from
Aug 4, 2023
Merged

Conversation

r4mmer
Copy link
Member

@r4mmer r4mmer commented Aug 3, 2023

Acceptance Criteria

  • Use node18 LST for build and deploy
  • Upgrade actions to most recent versions locking the git commit id of the release, not the tag name
  • Enable openssl legacy provider to enable build with node18

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

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@r4mmer r4mmer self-assigned this Aug 3, 2023
@r4mmer r4mmer requested review from tuliomir and glevco August 3, 2023 22:33
Copy link
Contributor

@tuliomir tuliomir left a 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",
Copy link
Contributor

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

Copy link
Member Author

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.

package.json Show resolved Hide resolved
@r4mmer r4mmer requested a review from tuliomir August 4, 2023 13:44
@r4mmer r4mmer merged commit cd0d5bc into dev Aug 4, 2023
1 check passed
@r4mmer r4mmer deleted the chore/remore-eol-node-versions branch August 4, 2023 15:17
msbrogli pushed a commit that referenced this pull request Aug 4, 2023
* chore: remove eol node versions from CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants