feat: allow access to UPnP client from types

[MichaelJCole]

…rts property

Title

feat(@libp2p/upnp-nat): Export types to access client and get openPorts property

Description

Hi, this PR exports types to access the upnp-nat client. This is the smallest change to access the openPorts map.

console.log(libp2p.services.upnp._getClient().openPorts)

Notes & open questions

My router has upnp bug reports in several forums. It's an old Frontier router and is being replaced. Sometimes @achingbrain/nat-port-mapper doesn't find the gateway (even when specified), or it responds with a 500 error in XML (I can't reproduce the error because now the gateways isn't found).

Anyways, it would be nice if @achingbrain/nat-port-mapper offered a "lastError" and/or "status" property that was accessible in libp2p. That's not what this PR does, this PR just provides types to access the openPorts property.

Change checklist

• [x ] I have performed a self-review of my own code
• I have made corresponding changes to the documentation if necessary (this includes comments as well)
• I have added tests that prove my fix is effective or that my feature works

[achingbrain]

Thanks for opening this.

This seems fair enough but I think exporting the class as a whole exposes too much of the internal API. For example it implements Startable - the user shouldn't be able to start/stop individual services outside of the general libp2p lifecycle start/stop methods as that way leads to chaos.

The underscore in ._getClient generally means 'private' or at least 'do not call'. It's a separate method because it used to return a promise though not any more - now I think we can just set the property in the constructor.

To get this in it needs:

1. Export a UPnPNAT interface from the index.js file:

// index.js
import { UPnPNAT as UPnPNATClass } from './upnp-nat.js'
import type { NatAPI } from '@achingbrain/nat-port-mapper'

export type { NatAPI }

export interface UPnPNAT {
  client: NatAPI
}

// ...other code here

export function uPnPNAT (init: UPnPNATInit = {}): (components: UPnPNATComponents) => UPnPNAT {
  return (components: UPnPNATComponents) => {
    return new UPnPNATClass(components, init)
  }
}

2. Have the UPnPNat class implement the interface:

// upnp-nat.ts
import type { UPnPNAT as UPnPNATInterface } from './index.js' 
// ...other code here

export class UPnPNAT implements Startable, UPnPNATInterface {
  // ...other code here
  public client: NatAPI

  constructor (components: UPnPNATComponents, init: UPnPNATInit) {
    // ...other code here
    this.client = upnpNat({
      //...
    }) 
  }
}

3. Replace use of this._getClient() with this.client in the UPnPNAT class

The UPnPNAT -> UPnPNATClass/UPnPNAT -> UPnPNATInterface rename dance is necessary to have the JavaScript name of the class be UPnPNAT and also the TypeScript interface name be UPnPNAT, it avoids appending Impl or other unpleasantness and is consistent with other modules here.

[MichaelJCole]

Hi Alex, thanks for your reply. I'm not sure how js-libp2p development is funded or organized. I am a freelancer, and I've worked on all kinds of projects from SMB to enterprise in all kinds of roles from solo contributor to tech lead.

I've noticed the js-libp2p codebase does a lot with trying to separate types and classes, and to public/private methods and properties. In my experience this might be useful in a mature project to package up the API, or in defense/data center applications, but it causes a lot of extra work and headaches for people trying to use software that is still being heavily developed.

Removing start/stop from the type only removes it from the vs-code typescript tooling. It's still there in the javascript code and anyone can still call it. Have you had alot of problems with users doing this? You could argue that neither of our proposed changes solves an actual problem. For example this workaround for getting the addressManager:

    // @ts-ignore missing libp2p.components in types
    const am = libp2p.components.addressManager as DefaultAddressManager

From what I can tell typescript's 'private' and 'readonly' are only enforced at compile time and you'd have to use #field for runtime private fields with a build target of ES2021 or later.

Contributing to js-libp2p looks like a lot of extra work, and mind-space, when what I really want to do is use it without problems to build something else. I don't see how adding another interface adds that much value here. Separating the type from the class makes the tooling harder to use.

js-libp2p might increase its velocity and get further, faster, if it wasn't adding abstractions and property declarations to close off functionality.

Anyways, maybe you have a large paid team and you're executing a plan I don't see?

My biggest concern is that js-libp2p doesn't have functionality I thought it did (e.g. direct webrtc connections), and the velocity of it's development.

Anyways, thanks for your time responding and all the work done so far.

[achingbrain]

I'm not sure how js-libp2p development is funded or organized.

js-libp2p gets occasional grants from the libp2p foundation, though it's long-term state is fairly precarious. There's a talk about it here.

It's organised via the weekly contributors calls and if you want to do a piece of work, you are welcome to pitch it to the foundation to try to get some funding to do that work.

Anyways, maybe you have a large paid team and you're executing a plan I don't see?

I'm afraid not - you can look at the last few months worth of contributions for who is working on what.

js-libp2p might increase its velocity and get further, faster, if it wasn't adding abstractions and property declarations to close off functionality.

I think it's the opposite - if all methods and properties are public all the time, then every interaction has to be considered and supported, which increases the maintenance burden on the project contributors.

Over time if it's desirable to expose certain pieces of internal functionality that can happen, but it has to happen in a manageable way.

For this PR, if all you need is access to the UPnP client, then that's all it should expose. Anything else can be done as and when required.