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

Support broader range of dependency versions #758

Draft
wants to merge 7 commits into
base: testing-min-dependencies
Choose a base branch
from

Conversation

cameel
Copy link
Member

@cameel cameel commented Jan 25, 2025

Depends on #757.
Replaces the version bump attempted in #756.

This PR extends the ranges of dependency versions marked as supported both ways:

  • Where possible I lowered the minimum. The new versions are the lowest ones that install, build and pass tests on all our supported node.js versions. There could still be some more subtle breakage not detected by tests but the same can be said for current versions - it's not like we chose them all that carefully. They're mostly whatever was the latest when we last bumped them. I checked history for the few cases where the bumps were clearly intentional and retained them.
  • I also relaxed upper limits so that we accept newer versions even across breaking upgrades. Most packages seem to be actually pretty widely compatible. There were only a few that weren't and I added restrictions only for those.

@cameel cameel requested a review from r0qs January 25, 2025 02:15
@cameel cameel self-assigned this Jan 25, 2025
"memorystream": "^0.3.1",
"semver": "^5.5.0",
"tmp": "0.0.33"
"command-exists": ">=1.1.0",
Copy link
Member Author

@cameel cameel Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I replaced ^ with >= in all version ranges. I'm still on the fence whether that's a good idea or whether I should always put a < there to keep major version updates manual:

Pros of >=:

  • We almost never update these versions and this forces projects using solc-js to use old versions too (or do hacky workarounds). >= forces the update on us by default and we can always restrict with < as needed.
  • There's not much risk of this seriously breaking a downstream app that's already using the package. If it happens, the app can always restrict the dependency in its own package.json. And that's only a concern for the devs of the app, not its users. It may only happen when devs update their dependencies - outside of that the installed versions are locked via package-lock.json.

Cons of >=:

  • Already released versions in npm will stay unlimited forever and eventually stop working with latest versions of dependencies because one of them will inevitably introduce an actual breaking change. This may confuse someone trying to add a dependency on an older version of solc-js to their app.
  • There's some risk of a new breaking version appearing during our release process. Though in solc-js this would be easy to fix quickly, because CI is quick and it gets tagged late in the process.

"semver": "^5.5.0",
"tmp": "0.0.33"
"command-exists": ">=1.1.0",
"commander": ">=8.0.0 <12.0.0",
Copy link
Member Author

@cameel cameel Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Versions >=13.0.0 do not pass our tests. We could probably support them if we wanted but that will require some code changes on our side. Not sure how extensive.

Versions >=12.0.0 trigger failures on older node.js versions due to the use of ?? operator or its variants.

Comment on lines +62 to +74
"@typescript-eslint/eslint-plugin": ">=5.0.0 <6.0.0",
"@typescript-eslint/parser": ">=5.0.0 <6.0.0",
"coveralls": ">=3.0.0",
"eslint": ">=7.12.1 <8.0.0",
"eslint-config-standard": ">=16.0.3 <17.0.0",
"eslint-plugin-import": ">=2.25.0",
"eslint-plugin-node": ">=11.1.0",
"eslint-plugin-promise": ">=5.0.0 <7.0.0",
"nyc": ">=15.0.0",
"tape": ">=4.11.0",
"tape-spawn": ">=1.0.0",
"ts-node": ">=10.0.0",
"typescript": ">=4.2.2 <5.0.0"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript, eslint and their plugins here are restricted only because they have issues on node.js 12 and 14. There are failures due to the use of ?? operator or its variants. On the other hand there are no issues on the latest node.js.

Not sure how to handle this. I guess node.js 12 and 14 are past EOL now so it may be ok to just remove the restriction. The issue is that our tests fail, so we would have to patch package.json in our tests.

Another possibility might be to just drop official v12 and v14 support.

@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Jan 25, 2025
@cameel cameel marked this pull request as draft January 25, 2025 03:19
@cameel cameel force-pushed the broader-dependency-version-ranges branch from 1fee900 to 284967f Compare January 25, 2025 03:46
@cameel cameel force-pushed the testing-min-dependencies branch from 06fdfd0 to 196f913 Compare January 25, 2025 03:46
@coveralls
Copy link

Coverage Status

coverage: 84.537%. remained the same
when pulling 284967f on broader-dependency-version-ranges
into 196f913 on testing-min-dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies The PR depends on other PRs that must be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants