-
Notifications
You must be signed in to change notification settings - Fork 483
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
Drop nodejs 10 #756
base: master
Are you sure you want to change the base?
Drop nodejs 10 #756
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.
Ok, let's do it. Node 10 is ancient at this point and I don't think it's essential. We try to support even very old versions mostly because it's not much effort for us. But if the broader ecosystem is dropping it, it's time to let it go.
Please remember to add a changelog entry in the main repo, like we did when we dropped nodejs v8. The changelog for solc-js is maintained there.
That's not how it works. We don't require specific versions of dependencies, we support whole ranges and it's the downstream app that chooses a single version from that range. On our end generally shouldn't have to bump anything as long as What we may need to do from time to time is to update to breaking versions, but that's not a simple version bump. It will usually require changes in code, otherwise the version would not be breaking. It may sometimes happen that the breaking changes do not affect our usage, but we still need to verify that it's the case. And it just means that we can broaden the version range without dropping support for the older one. BTW, support for older versions is something we should be testing. Our tests run with latest versions of dependencies, but we should also have one with oldest allowed ones to notice when we're introducing something that does not work with them.
Feel free to do that update in a separate PR. Just make sure we're not restricting them too much. Like, we don't necessarily want to update if frameworks are still mostly on an older, incompatible version. Whenever possible and we can work with both old and new version, just add mark the newer version as supported without actually dropping the older one.
How would a Or do you mean that we should actually restrict the library to one specific version of each dependency? I don't think that's a good idea. |
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, do we even need to bump these? They all use ^
so doing that is not necessary to get newer versions. Instead, what we're doing is just to preventing the older ones from being installed. Are these older versions known to be broken?
I think bumping dependency versions is not necessary
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.
By the way, you should also update this bit:
"engines": {
"node": ">=10.0.0"
},
BTW, I see that the latest node.js version is now 23 so we should add a run on 22. |
This PR removes Node.js 10 from our CI as it started failing due to many dependencies increasing their Node.js version requirements. See: https://app.circleci.com/pipelines/github/ethereum/solc-js/1775/workflows/14eb5b72-7624-47e5-a302-a407c1fd4f58/jobs/11376/parallel-runs/0?invite=true#step-105-0_136 and this error: https://app.circleci.com/pipelines/github/ethereum/solc-js/1775/workflows/14eb5b72-7624-47e5-a302-a407c1fd4f58/jobs/11376?invite=true#step-108-209_118, which is because
Object.fromEntries
is only available in Node.js 12+. We could potentially workaround the issue usingDISABLE_V8_COMPILE_CACHE=1
, but I believe that it makes more sense to update the packages.Also, ESLint v8 dropped support for Node.js 10, as noted here eslint/eslint#15560 (comment).
As a result, we should probably consider bumping everything to align with the newer Node.js versions. And we might need to revisit the suggestion from #723 to lock our dependencies for better consistency.
Note, I decided to bump some versions, but not the major ones yet.