-
Notifications
You must be signed in to change notification settings - Fork 482
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # 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. |
I think bumping dependency versions is not necessary
BTW, I see that the latest node.js version is now 23 so we should add a run on 22. |
Ah, got it. Yeah, it indeed does make sense to support a range of dependencies.
My original comment was motivated by concerns raised in #570, rather than by the specific versions set in #723. However, after revisiting the issue, I agree with you, and it does seem preferable to keep things up to date and support a range of versions rather than fixing them. Should we then consider those issue and PR closed? |
3017192
to
ef7001a
Compare
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.