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

Drop nodejs 10 #756

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Drop nodejs 10 #756

wants to merge 2 commits into from

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Jan 23, 2025

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 using DISABLE_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.

@r0qs r0qs requested a review from cameel January 23, 2025 16:16
cameel
cameel previously approved these changes Jan 24, 2025
Copy link
Member

@cameel cameel left a 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.

@cameel
Copy link
Member

cameel commented Jan 24, 2025

As a result, we should probably consider bumping everything to align with the newer Node.js versions.

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 package.json is written correctly. Version range should be open-ended, allowing new versions as long as they're compatible according to semver.

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.

Note, I decided to bump some versions, but not the major ones yet.

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.

And we might need to revisit the suggestion from #723 to lock our dependencies for better consistency.

How would a package-lock.json help with anything? It's just a snapshot of dependency versions at a random point in time. It also does not mean anything to us, because we still support the whole range from package.json, not just the set from the lock file. IMO testing latest and oldest combinations of versions would be much more helpful.

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.

Copy link
Member

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?

@cameel cameel dismissed their stale review January 24, 2025 19:24

I think bumping dependency versions is not necessary

Copy link
Member

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"
  },

@cameel
Copy link
Member

cameel commented Jan 25, 2025

BTW, I see that the latest node.js version is now 23 so we should add a run on 22.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants