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

deps(@libp2p/websockets): upgrade multiaddr-to-uri to parse tls addrs #2059

Conversation

danisharora099
Copy link

@danisharora099 danisharora099 commented Sep 18, 2023

Closes #2024

@danisharora099 danisharora099 requested a review from a team as a code owner September 18, 2023 17:12
@maschad maschad changed the title chore: upgrade multiaddr-to-uri & multiformats/* deps(@libp2p/websockets): upgrade multiaddr-to-uri to parse tls addrs Sep 18, 2023
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

Thanks!

@achingbrain
Copy link
Member

These are all in-range semver updates, if you are not seeing these versions when you install @libp2p/websockets, please delete your lockfiles.

@maschad
Copy link
Member

maschad commented Sep 18, 2023

Whilst I agree @achingbrain I still think it's valid to set @multiformats/multiaddr-to-uri as a version higher than 9.0.5 given the fix that introduced support for tls addrs.

Consumers who upgrade to @libp2p/websockets 7.0.7 or later will not have to be told to delete their lockfile, and it's routine to upgrade a package when encountering an issue.

@achingbrain
Copy link
Member

Updating minors for every dependency is busywork that creates needless release churn.

This is precisely what semver ranges are supposed to prevent.

@maschad
Copy link
Member

maschad commented Sep 18, 2023

Actually this PR is upgrading patch versions - but @multiformats/multiaddr-to-uri should have had a minor upgrade i.e. from 9.0.4 -> 9.1.0 since that fix added new backwards compatible functionality to multiaddrToUri, but that was an oversight, and so this is an extenuating circumstance. I agree creating PRs to upgrade patch versions should not be a standard practice.

@achingbrain
Copy link
Member

but @multiformats/multiaddr-to-uri should have had a minor upgrade i.e. from 9.0.4 -> 9.1.0

This would still have been an in-range semver update.

@maschad
Copy link
Member

maschad commented Sep 18, 2023

That's true, I forgot that Caret Ranges only presume breaking changes in minor upgrades for packages versioned less than 1.0.0

@maschad maschad closed this Sep 18, 2023
danisharora099 added a commit to waku-org/js-waku that referenced this pull request Sep 19, 2023
danisharora099 added a commit to waku-org/js-waku that referenced this pull request Sep 19, 2023
danisharora099 added a commit to waku-org/js-waku that referenced this pull request Sep 19, 2023
danisharora099 added a commit to waku-org/js-waku that referenced this pull request Sep 22, 2023
* add a test to dial tls version of a multiaddr

* generate new lockfile
ref: libp2p/js-libp2p#2059 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: missing support for /tls multiaddr
3 participants