Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

deps(core): semver@7.5.2->7.5.4 #6177

Merged
merged 15 commits into from
Sep 6, 2023
Merged

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Sep 3, 2023

PR description

  • bump direct dependency semver to latest 7.5.4.
  • bump direct dependency @types/semver to latest 7.5.1.
    • add assertion for type update
  • dedupe semver to address ReDoS in transitive deps.
  • dedupe @types/semver

Note that this does not completely remove all dependencies on broken versions of semver. It's still being pulled in via nx and [ethereumjs-block,ethereumjs-vm] > merkle-patricia-tree > levelup@1.

Testing instructions

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

  • I have added any needed breaking-change and new-feature labels for the appropriate packages.

@legobeat legobeat marked this pull request as ready for review September 3, 2023 21:21
@legobeat

This comment was marked as resolved.

@gnidan
Copy link
Contributor

gnidan commented Sep 3, 2023

Maintainers: This PR still keeps the version pinned to conform with existing approach but I'd like to propose changing to caret range ^7.5.4 for direct dependencies on semver to reduce further churn for you and downstreams for any future bugfixes.

I don't think we pin semver for any particular reason, so we can probably just switch it to caret. I'll check the history to confirm next week, to see if we pinned it on purpose at some point.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Seems good to me!

@haltman-at
Copy link
Contributor

haltman-at commented Sep 5, 2023

Also, I just looked through the history and it looks to me like it just got pinned mistakenly as part of #5309 and none of us noticed. I don't see any particular reason it got pinned. I'm OK with this being merged in its current form, but I agree it would be preferable for them to be unpinned.

@legobeat
Copy link
Contributor Author

legobeat commented Sep 5, 2023

Also, I just looked through the history and it looks to me like it just got pinned mistakenly as part of #5309 and none of us noticed. I don't see any particular reason it got pinned. I'm OK with this being merged in its current form, but I agree it would be preferable for them to be unpinned.

@haltman-at Got it, I unrestricted it in 5f91fce

Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Confirmed that pinning seems accidental. Thanks for making this change!

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

Successfully merging this pull request may close these issues.

3 participants