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

switch to @chainsafe/is-ip (away from @achingbrain/ip-address) #1926

Closed
SgtPooki opened this issue Aug 3, 2023 · 2 comments · Fixed by #1957
Closed

switch to @chainsafe/is-ip (away from @achingbrain/ip-address) #1926

SgtPooki opened this issue Aug 3, 2023 · 2 comments · Fixed by #1957
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up

Comments

@SgtPooki
Copy link
Member

SgtPooki commented Aug 3, 2023

Is there a reason why we're still using @achingbrain/ip-address, or even ip-address, when it's outdated? There seem to be better and more up to date alternatives: https://npmtrends.com/ip-vs-ip-address-vs-ipaddr.js. Though maybe they don't do everything we need them to?

One of ip-address 's dependencies, jsbn, is very stale and very underused in comparison to it's competitors: https://npmtrends.com/bignumber.js-vs-bn-vs-bn.js-vs-jsbn

There is also the native BigInt which has been around for a while: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt

We only seem to have a very small surface area for each here:

I also seem to remember some concerns around node-forge being used in js-libp2p and this may be another place where we could reduce our ties to it.


I'm not aware of the nuances but generally if a project leaves a PR that we need (beaugunderson/ip-address#148) untouched for over a year (it's going on 2), and we still don't have a permanent place to modify code for our patched version, we should probably think about migrating, or publishing our version.

cc @achingbrain. related to ipfs/helia#73 (comment)

@SgtPooki SgtPooki added the need/triage Needs initial labeling and prioritization label Aug 3, 2023
@SgtPooki
Copy link
Member Author

SgtPooki commented Aug 3, 2023

w.r.t. BigInt we're using https://www.npmjs.com/package/protons, which uses BigInt as well

@achingbrain
Copy link
Member

achingbrain commented Aug 3, 2023

@achingbrain/ip-address is just ip-address with beaugunderson/ip-address#148 applied.

is there a reason why we're still using [insert module name here]

Not generally no, outside of "it works" and "it doesn't cause enough problems to bring eyeballs to it".

Since this is now starting to cause problems I think we can switch it out for @chainsafe/is-ip which we use elsewhere in the stack.

@maschad maschad added this to js-libp2p Aug 3, 2023
@maschad maschad moved this to 🛠️ Todo in js-libp2p Aug 3, 2023
@maschad maschad added kind/maintenance Work required to avoid breaking changes or harm to project's status quo and removed need/triage Needs initial labeling and prioritization labels Aug 3, 2023
@p-shahi p-shahi changed the title bug: using stale ip-address and jsbn switch to @chainsafe/is-ip (away from @achingbrain/ip-address) Aug 8, 2023
@p-shahi p-shahi added help wanted Seeking public contribution on this issue exp/novice Someone with a little familiarity can pick up P3 Low: Not priority right now P2 Medium: Good to have, but can wait until someone steps up and removed P3 Low: Not priority right now labels Aug 8, 2023
achingbrain added a commit that referenced this issue Aug 11, 2023
…ddress

The `@achingbrain/ip-address` module is a fork of `ip-address` which
seems to have dependencies on older modules that break modern runtimes
like react native.

`@Chainsafe/is-ip` does not have these limitations so switch to that.

Fixes #1926
achingbrain added a commit that referenced this issue Aug 13, 2023
The `@achingbrain/ip-address` module is a fork of `ip-address` which
seems to have dependencies on older modules that break modern runtimes
like react native.

`@Chainsafe/is-ip` does not have these limitations so switch to that.

Fixes #1926
@github-project-automation github-project-automation bot moved this from 🛠️ Todo to 🎉Done in js-libp2p Aug 13, 2023
This was referenced Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up help wanted Seeking public contribution on this issue kind/maintenance Work required to avoid breaking changes or harm to project's status quo P2 Medium: Good to have, but can wait until someone steps up
Projects
Archived in project
4 participants