fix: typedefs for MulticodecTopology

[Gozala]

Without this change ipfs/js-ipfs-bitswap#261 fails type-check with a following error:

src/network.js:50:26 - error TS2351: This expression is not constructable.
  Type 'MulticodecTopology' has no construct signatures.

50     const topology = new MulticodecTopology({
                            ~~~~~~~~~~~~~~~~~~


Found 1 error.

That is because typedefs claim that exported value is an instance of MulticodecTopology while it actually exports the class. min and max are also optionals.

[vasco-santos]

This needs to be fixed in the JS file. This module is using its npm run generate:types script to generate the types while do not updated to the new aegir

[hugomrdias]

better to just update to the new ts cmd in aegir

[vasco-santos]

better to just update to the new ts cmd in aegir

I am working on updating that as we also need to create declarations for interfaces like Transport. So, the easy way is to unblock bitswap sooner is to keep this setup for now

[Gozala]

I have made following changes:

1. Removed class-is dependency (we already removed in few other places) because it doesn't really add much and here it was reason why TS was unable to do a type inference.

   I tried to change type annotation for module.exports from @type {MulticodecTopology} to @type {typeof MulticodecTopology} but that introduces other TS issues, because TS want's original classes to be exported.

2. I have set tsconfig to strict (which is what we have in new aegir config too), because without this setting optional fields aren't getting expected types (e.g. instead of min?: number we get min: number because in non strict setup everything is treated as optional).
3. Added few missing type imports that lead to issues with strict setting.
4. Regenerated the types with existing npm script.
5. Disabled some jsdoc eslint rules in few files, because older aegir used here uses plugin that is does not recognize some type annotations used here.

[vasco-santos]

Thanks for these changes @Gozala

The class-is has also been problematic for me trying to integrate the transport interface and the connection. So, I agree we should follow what has been done and remove it.

The changes seem good to me, but there are a few pubsub things that we should change. I tried out this branch with https://github.com/ChainSafe/js-libp2p-gossipsub which uses this interface types and got some problems:

ts/index.ts:69:7 - error TS2415: Class 'Gossipsub' incorrectly extends base class 'PubsubBaseProtocol'.
  Property '_addPeer' is private in type 'PubsubBaseProtocol' but not in type 'Gossipsub'.

69 class Gossipsub extends Pubsub {
         ~~~~~~~~~

ts/index.ts:146:11 - error TS2345: Argument of type '{ scoreParams: PeerScoreParams; scoreThresholds: PeerScoreThresholds; emitSelf: boolean; gossipIncoming: boolean; fallbackToFloodsub: boolean; ... 14 more ...; libp2p: Libp2p; }' is not assignable to parameter of type '{ debugName: string; multicodecs: string | string[]; libp2p: any; globalSignaturePolicy: { StrictSign: "StrictSign"; StrictNoSign: string; } | undefined; canRelayMessage: boolean | undefined; emitSelf: boolean | undefined; }'.
  Property 'canRelayMessage' is missing in type '{ scoreParams: PeerScoreParams; scoreThresholds: PeerScoreThresholds; emitSelf: boolean; gossipIncoming: boolean; fallbackToFloodsub: boolean; ... 14 more ...; libp2p: Libp2p; }' but required in type '{ debugName: string; multicodecs: string | string[]; libp2p: any; globalSignaturePolicy: { StrictSign: "StrictSign"; StrictNoSign: string; } | undefined; canRelayMessage: boolean | undefined; emitSelf: boolean | undefined; }'.

146     super({
              ~
147       debugName: 'libp2p:gossipsub',
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
150       ...opts
    ~~~~~~~~~~~~~
151     })
    ~~~~~

  node_modules/libp2p-interfaces/src/pubsub/index.d.ts:38:9
    38         canRelayMessage: boolean | undefined;
               ~~~~~~~~~~~~~~~
    'canRelayMessage' is declared here.

ts/index.ts:301:21 - error TS2341: Property '_addPeer' is private and only accessible within class 'PubsubBaseProtocol'.

301     const p = super._addPeer(peerId, protocol)
                        ~~~~~~~~

ts/index.ts:328:31 - error TS2341: Property '_removePeer' is private and only accessible within class 'PubsubBaseProtocol'.

328     const peerStreams = super._removePeer(peerId)

The errors seem related to _addPeer, _ removePeer being private now (they are extended in gossipsub), as well as the option canRelayMessage not being declared in gossipsub and lost the optional sign.

These things can also be updated in gossipsub, but I was trying to avoid a breaking change at this moment, until we finalize the types for the interface.

What do you think? Can we easily revert this pubsub types without blocking bitswap?

[Gozala]

What do you think? Can we easily revert this pubsub types without blocking bitswap?

I don't think those are changes that I've made it's just new typescript is recognizing @private tags that were there. I might be mistaken but changing them to @protected might resolve these. We could also remove @private for now.

[vasco-santos]

It should be because of the build. Also not sure, but if protected solves this issue, let's go with it for now 👍

[Gozala]

@vasco-santos I have changed all @private members to @protected. Former can be accessed only from the class itself while later can be accessed from class or it's subclasses, which is what seems to be desired here.

I have also created a type definition for the Pubsub options so that optional fields are preserved as optionals instead of been turned into T|undefined which seems to happen for inline param definitions.

[vasco-santos]

@vasco-santos I have changed all @Private members to @Protected. Former can be accessed only from the class itself while later can be accessed from class or it's subclasses, which is what seems to be desired here.

This solves the functions issue, thanks 👌

I have also created a type definition for the Pubsub options so that optional fields are preserved as optionals instead of been turned into T|undefined which seems to happen for inline param definitions.

Getting these two errors now. Seem related to that, right?

> libp2p-gossipsub@0.7.0 pretest /Users/vsantos/work/pl/gh/libp2p/gossipsub-js
> tsc

ts/index.ts:146:11 - error TS2345: Argument of type '{ scoreParams: PeerScoreParams; scoreThresholds: PeerScoreThresholds; emitSelf: boolean; gossipIncoming: boolean; fallbackToFloodsub: boolean; ... 14 more ...; libp2p: Libp2p; }' is not assignable to parameter of type 'Options'.
  Types of property 'globalSignaturePolicy' are incompatible.
    Type 'string' is not assignable to type '"StrictSign" | "StrictNoSign" | undefined'.

146     super({
              ~
147       debugName: 'libp2p:gossipsub',
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
150       ...opts
    ~~~~~~~~~~~~~
151     })
    ~~~~~

ts/index.ts:353:5 - error TS2322: Type 'PeerStreams | undefined' is not assignable to type 'PeerStreams'.
  Type 'undefined' is not assignable to type 'PeerStreams'.

353     return peerStreams
        ~~~~~~~~~~~~~~~~~~


Found 2 errors.

Other than that, did we have any decision to put typedef on the end of the file? I remember seeing that discussion on a PR and I thought we would go with the typedefs on top of the file. I also synced with jacob about following that pattern in libp2p land

[Gozala]

Other than that, did we have any decision to put typedef on the end of the file?

I believe we have converged on putting them at the end instead of top. More details on the proposed approach we plan on using is here ipfs/js-ipfs#3413

[vasco-santos]

Done in #74