chore: add typedefs #802

vasco-santos · 2020-11-16T11:10:50Z

This PR adds typedefs in most of the core codebase. Libp2p interfaces was updated to leverage its new types.

Github actions replaced travis, which also enables us to just use it for windows 🎉

This brings a set of advantages:

enable us to generate typescript declaration files automatically on release with aegir.
Intellisense for libp2p users
- Users will be able to see on the fly the libp2p API, expectations for each functions and associated docs/examples
- Example
Intellisense for libp2p core team
- Core team will be able to see internal types and expectations of each functions without the need to fly around the code. It also enables other editors features like jump to definition, or pop ups with implementation of a highlighted function
Automated docs
- As a follow up, we should be able to automatically generate the API docs

Follow up work: #830

Needs:

feat: add types js-libp2p-interfaces#74
At the moment, it is using a branch from the PR below that has a prepare script to generate the build while we do not release it.

jacobheun

enable us to generate typescript declaration files automatically on release with aegir.

Will we get these automatically now, do we need to update anything for the release?

What are we missing / What is next?

For what's missing, has this been checked against any downstream TS projects? The intellisense is nice, but if we're going to be generating the types on release we should make sure to test this on a larger existing project. For example, can we swap out the types here in Lodestar, https://github.com/ChainSafe/lodestar/tree/master/packages/lodestar, for the types at https://github.com/ChainSafe/libp2p-ts.git that it's using in dev?

jacobheun · 2020-11-16T13:56:32Z

src/peer-store/book.js

@@ -30,7 +29,7 @@ class Book {
    /**
     * Map known peers to their data.
     *
-     * @type {Map<string, Array<Data>}
+     * @type {Map<string, Array<*>}


Instead of any, *, should these be generics (T)? Based on how the books are structured the data type should be consistent for each book.

You can make Book generic by adding @template T above class definition. That would also make options.eventTransformer bounded.

src/insecure/plaintext.js

jacobheun · 2020-11-16T14:05:29Z

src/identify/index.js

@@ -290,7 +264,7 @@ class IdentifyService {
   *
   * @private
   * @param {object} options
-   * @param {*} options.stream
+   * @param {DuplexIterable} options.stream


We should probably track adding a type for Stream in the interfaces module, that should probably live with Connection. While any DuplexIterable should be fine, we really want these to be Stream's

vasco-santos · 2020-11-16T15:04:45Z

Will we get these automatically now, do we need to update anything for the release?

Pending ipfs/aegir#671 for support on release. But yes, I can do that test by that time

Gozala

Just added few nits & general feedback

src/address-manager/index.js

Gozala · 2020-11-16T21:33:54Z

src/circuit/auto-relay.js

+ */
+
+/**
+ * @typedef {Object} AutoRelayProperties


Nit: In js-ipfs we settled on calling injected deps with Config suffix e.g. AutoRelayConfig`. Might be worth following same convention across both code bases.

Probably mentioning in the one below, right?
The current approach is: xProperties to libp2p references needed and xOptions for user options, so that we can use xOptions on the top level config docs. We can definitely change xOptions to xConfig and I would be happy in having a better name for xProperties if you have any suggestions

Bo I was referring to xProperties actually, maybe I’ve misread the code, but it looked like dependencies injected into AutoRelay and we called such things with xConfig e.g.:

https://github.com/ipfs/js-ipfs/blob/530767d85d85b5932abdb87721e395c0a214b148/packages/ipfs-core/src/components/dag/get.js#L8-13

I would like to get a good way for here. So, in the libp2p stand point we have lots of places where we inject libp2p, or some libp2p components on other libp2p components. Moreover, these libp2p components have configurations that users can modify.
We need to differentiate between them. My initial approach was to make ComponentNameProperties for the libp2p components we provide, and ComponentNameOptions for user options. Naming config for dependencies that are not "configurable" does not seem the best naming to me. However, I am not totally happy with xProperties, so if we can find a better convention would be great

Maybe AutoRelayComponents then ?

The reason I end up calling those config in js-ipfs is because it is configuration of the component. It is true that user can't modify those but who ever creates a component can provide alternative configuration e.g. in memory repo vs on disk repo etc...

Things that user can configure are referred as PrefixOptions.

I don't have strong feeling about naming convention itself, just would love to settle on the same convention across ipfs and libp2p

Components seems a good alternative. Will wait for other people to weight in before renaming everywhere

I don’t think we should block this due to naming convention. Lets just have a separate discussion thread somewhere instead and we can always come back and rename things.

yeah I agree. But let's keep the discussion open until hugo/jacob comments

src/content-routing.js

Gozala · 2020-11-16T21:48:58Z

src/peer-store/book.js

@@ -30,7 +29,7 @@ class Book {
    /**
     * Map known peers to their data.
     *
-     * @type {Map<string, Array<Data>}
+     * @type {Map<string, Array<*>}


You can make Book generic by adding @template T above class definition. That would also make options.eventTransformer bounded.

vasco-santos · 2020-11-25T17:03:39Z

The first batch of review comments were addressed. I will test this with lodestar over the week.

Meanwhile, @Gozala created https://www.npmjs.com/package/typed-events.ts for the EventEmitter stuff ❤️ .
After syncing with him, we might want to get the declaration file from there in and use https://www.npmjs.com/package/proper-event-emitter per #761 (we also have emittery and some modules and we need to double check ig we should just use it). Accordingly, I think we can land the typedefs for EventEmitter in a new PR to not block this.

… changes

vasco-santos · 2020-12-03T16:43:08Z

@hugomrdias @Gozala @jacobheun

This is ready and already tested successfully with lodestar build.

Unfortunately, it seems that we will not be able to use templates in the PeerStore Book and I needed to add any to the data types for now (more info on: #821 -- I will create an issue in typescript as the aegir type check works ok with #821 templates, but then the *.ts.d are not correctly generated which breaks user builds)

Gozala

Thanks @vasco-santos this must have been a lot of work & I can’t wait for this to lang and propagate across code base 🤩

I added bunch of minor comments and called out few instances where use of generics seems more appropriate. I think it’s mostly there, I just would really like type-check action to get uncommented to ensure all of this type-checks.

.github/workflows/main.yml

src/address-manager/index.js

Gozala · 2020-12-09T07:05:03Z

src/circuit/auto-relay.js

+ */
+
+/**
+ * @typedef {Object} AutoRelayProperties


I don’t think we should block this due to naming convention. Lets just have a separate discussion thread somewhere instead and we can always come back and rename things.

src/circuit/circuit/hop.js

src/circuit/circuit/stop.js

Gozala · 2020-12-09T08:15:26Z

src/metrics/stats.js

@@ -68,7 +71,7 @@ class Stats extends EventEmitter {
  /**
   * Returns a clone of the current stats.
   *
-   * @returns {Map<string, Stat>}
+   * @returns {Object}


Suggested change

* @returns {Object}

* @returns {Record<string, Stat>}

Assuming Stat is defined somewhere.

Stat is not defined yet. Metrics still have a lot of work to be done as follow up and as this is internal code I did not prfioritize getting through all this now.

src/peer-store/address-book.js

Gozala · 2020-12-09T08:55:56Z

src/peer-store/book.js

 class Book {
  /**
+   * The Book is the skeleton for the PeerStore books.


Seems like Book here supposed to be generic with Data type parameter e.g.

/** * @template Data */ class Book { set(peerId:PeerId, data:Data[]|Data):void get(peerId:PeerID):Data[]|undefined ... }

Yes, it should probably have a composed template. I started doing it and it was working good here. However, when I tried to integrate this in lodestar, I was getting issues on building their project. You have more information on this in #802 (comment) . I have it done in #821 and I already talked with @hugomrdias about it. It seems like a typescript issue (I need to open an issue there), and we should track it before changing it here.

I have looked into this & turns out this was different issue fixed in typescript@4.1.x. So, I have submitted #831 against #821, which addresses this problem and fixes few type miss matches that have came up.

I think it would be good to take a look at that pull as it also uncovered some issues that I have missed when reviewing this.

src/ping/index.js

Gozala · 2020-12-09T09:10:29Z

src/record/peer-record/index.js

-   * @param {Record} other
-   * @returns {boolean}
+   * @param {PeerRecord} other
+   * @returns {other is Record}


Since other is already required to be a PeerRecord instance which implements Record interface it is granted so this return type will have no effect.

Suggested change

* @returns {other is Record}

* @returns {boolean}

It errors with:

Property 'equals' in type 'PeerRecord' is not assignable to the same property in base type 'Record'. Type '(other: PeerRecord) => boolean' is not assignable to type '(other: any) => other is Record'. Signature '(other: PeerRecord): boolean' must be a type predicate.

Per https://github.com/libp2p/js-libp2p-interfaces/blob/feat/add-types/src/record/types.ts#L20

Should I just change the param to any?

Ok so there are couple of things here:

That error occurs because interface says you can pass anything to the method, but then implementation contradicts it by saying by saying you only pass PeerRecord, implying that you can't pass number while interface says you can do that. In other words you can accept wider range of inputs (superset types) in implementations than type / interface requires and be compatible, but you can not do other way around.

I'm honestly confused by equals(other: any): other is Record. Using other is Type kind of annotations are only useful for predicates that are meant to be used for type refinements e.g. example illustrating this

interface Duck { quack(): void } interface Dog { bark(): void } const isDuck = (value:any):value is Duck => value && typeof value.quack === 'function' export const workingExample = (value:Dog|Duck) => { if (isDuck(value)) { // In this block type of value is `Duck` value.quack() } if (value && typeof value.quack === "function") { // ^^^^^ Property 'quack' does not exist on type 'Duck | Dog'. value.quack() // ^^^^^ Property 'quack' does not exist on type 'Duck | Dog'. } }

Note that in some cases on might restrict type of value in predicate in some way (does not need to be any), but the point is that TS essentially treats such predicate functions to do type refinements.

I am not sure why one would want to do type refinement with equals check e.g. because if type of value in value.equals(other) is known and it's equal to other than value could be used in it's place e.g.:

const example = (duck:Duck, other:Duck|Dog) { if (duck.equals(other)) { duck.quack() // as opposed to other.quack() } }

I think you may want to use Record for parameter type, as in it does not matter if PeerRecord implementation is used or some compatible OtherRecord implementation.

I think generally it is best to avoid any unless you want to tell TS to just shut up about that value. That is because in that case TS will not complain no matter you do with it. I think in most cases actual intent is to say that value is something of any type (via generics interface Record { equals <T>(other:T):boolean } or more simply that value is of unknown type (via interface Record { equals(other:unknown):boolean }). This inspired me to write (what I hope is fun) explainer

If I had to guess you either want equals(other:unknown): boolean or bit more constrained equals(other:Record):boolean in both interface and a class.

Thanks for the example and detailed go through. I am changing the interface + here to unknown and leverage the instance of

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

Gozala · 2020-12-09T15:58:26Z

Unfortunately, it seems that we will not be able to use templates in the PeerStore Book and I needed to add any to the data types for now (more info on: #821 -- I will create an issue in typescript as the aegir type check works ok with #821 templates, but then the *.ts.d are not correctly generated which breaks user builds)

I think the issue you may be referring to is microsoft/TypeScript#41258 which got fixed just yesterday. But in the meantime there is also a workaround that projects can use:
https://github.com/multiformats/js-multiformats#typescript-support

vasco-santos · 2020-12-09T16:39:24Z

I think the issue you may be referring to is microsoft/TypeScript#41258 which got fixed just yesterday.

Thanks! I think it is that! ❤️ Sadly, they did not release it yet.

But in the meantime there is also a workaround that projects can use:
https://github.com/multiformats/js-multiformats#typescript-support

I feel it is better to add that as a follow up PR. Seems odd to me to release 0.30 with type definitions for TypeScript as one of the headlines, but then people need to skipLibCheck it. I would rather get it in when the new typescript is released as a patch.

What do you think? Any concerns on this approach? From the user stand point, not having template specified in the book yet will not have influence as they will use the book implementations that export the correct types.

Gozala

What do you think? Any concerns on this approach? From the user stand point, not having template specified in the book yet will not have influence as they will use the book implementations that export the correct types.

src/circuit/circuit/hop.js

src/circuit/circuit/stop.js

src/index.js

Gozala · 2020-12-09T23:49:19Z

src/index.js

@@ -583,7 +645,9 @@ class Libp2p extends EventEmitter {

    // Transport modules with discovery
    for (const Transport of this.transportManager.getTransports()) {
+      // @ts-ignore Transport interface does not include discovery


Submitted a followup issue #829

src/index.js

Gozala · 2020-12-09T23:56:51Z

src/keychain/cms.js

@@ -1,3 +1,4 @@
+// @ts-nocheck


Created #830 for that.

src/circuit/transport.js

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

vasco-santos requested a review from jacobheun November 16, 2020 11:36

jacobheun reviewed Nov 16, 2020

View reviewed changes

Gozala reviewed Nov 16, 2020

View reviewed changes

vasco-santos force-pushed the chore/add-typedefs branch from 26014cd to 1c54160 Compare November 25, 2020 14:36

vasco-santos mentioned this pull request Nov 25, 2020

chore: add duplex iterable type to connection libp2p/js-libp2p-interfaces#71

Closed

vasco-santos added 3 commits November 26, 2020 19:18

chore: add typedefs

26c40c8

chore: address review

7e05c4a

chore: add types checker script and more fixes on the jsdocs

28b9f85

vasco-santos force-pushed the chore/add-typedefs branch from 627486d to 28b9f85 Compare November 26, 2020 18:24

This was referenced Nov 27, 2020

⚡️ 0.32 RELEASE 🚀 #816

Closed

peerRouting.findPeer() does not accept strings, only PeerId instances #819

Closed

chore: use new libp2p interface

4b309ad

vasco-santos mentioned this pull request Dec 1, 2020

feat: add types libp2p/js-libp2p-interfaces#74

Merged

1 task

chore: minor fixes and github ci

149c19a

vasco-santos force-pushed the chore/add-typedefs branch from a674e4a to 149c19a Compare December 2, 2020 17:22

vasco-santos added 4 commits December 2, 2020 21:39

chore: fix remaining ts ignores

d3d6b35

chore: reset cache ci...

89fd524

chore: remove interfaces dep for ci cache

2109ef0

chore: reset cache ci...

32dd055

vasco-santos requested review from hugomrdias and Gozala December 2, 2020 21:01

vasco-santos force-pushed the chore/add-typedefs branch from 6a56dac to ded2a88 Compare December 3, 2020 10:42

chore: update record, transport and stream muxer per latest interface…

a5f5160

… changes

vasco-santos force-pushed the chore/add-typedefs branch 5 times, most recently from 2b76df2 to 265bf48 Compare December 3, 2020 11:26

vasco-santos force-pushed the chore/add-typedefs branch from 74475c1 to e519a6c Compare December 3, 2020 15:46

chore: minor changes from lodestar testing

55910c8

vasco-santos force-pushed the chore/add-typedefs branch from e519a6c to 55910c8 Compare December 3, 2020 16:00

chore: add libp2p modules typedef

1992609

Gozala suggested changes Dec 9, 2020

View reviewed changes

vasco-santos mentioned this pull request Dec 9, 2020

Support for Typescript users #659

Closed

chore: apply suggestions from code review

9b863bd

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

vasco-santos force-pushed the chore/add-typedefs branch from eb5b4dc to 9b863bd Compare December 9, 2020 11:08

gbaranski mentioned this pull request Dec 9, 2020

[libp2p] Add types DefinitelyTyped/DefinitelyTyped#50017

Closed

12 tasks

chore: address rest of review

b221e97

vasco-santos requested a review from Gozala December 9, 2020 18:19

Gozala mentioned this pull request Dec 9, 2020

Followup: Types #830

Closed

7 tasks

Gozala approved these changes Dec 10, 2020

View reviewed changes

Gozala reviewed Dec 10, 2020

View reviewed changes

src/circuit/transport.js Outdated Show resolved Hide resolved

Gozala mentioned this pull request Dec 10, 2020

Chore/add typedefs peerstore book template 2 #831

Closed

vasco-santos and others added 4 commits December 10, 2020 11:25

chore: apply suggestions from code review

9966c6c

Co-authored-by: Irakli Gozalishvili <contact@gozala.io>

chore: address other review comments

aa98bc2

chore: move create to static function

f568f16

chore: use new libp2p interface

ed2d10a

vasco-santos merged commit a279926 into 0.30.x Dec 10, 2020

vasco-santos deleted the chore/add-typedefs branch December 10, 2020 14:01

vasco-santos added a commit that referenced this pull request Dec 10, 2020

chore: add typedefs (#802)

64124f1

vasco-santos mentioned this pull request Dec 10, 2020

⚡️ 0.30 RELEASE 🚀 #655

Closed

17 tasks

hugomrdias mentioned this pull request Dec 11, 2020

Track: Typescript types support ipfs/js-ipfs#2945

Closed

vasco-santos added a commit that referenced this pull request Dec 16, 2020

chore: add typedefs (#802)

169bb80

vasco-santos mentioned this pull request Jan 7, 2021

Can I rewrite this thing in TypeScript? #858

Closed

vasco-santos mentioned this pull request Jan 15, 2021

Question: Key type returned by crypto.keys.unmarshalPublicKey libp2p/js-libp2p-crypto#184

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add typedefs #802

chore: add typedefs #802

vasco-santos commented Nov 16, 2020 •

edited

Loading

jacobheun left a comment

jacobheun Nov 16, 2020

Gozala Nov 16, 2020

jacobheun Nov 16, 2020

vasco-santos commented Nov 16, 2020

Gozala left a comment

Gozala Nov 16, 2020

vasco-santos Nov 17, 2020

Gozala Nov 17, 2020

vasco-santos Nov 25, 2020

Gozala Nov 26, 2020

vasco-santos Dec 2, 2020

Gozala Dec 9, 2020

vasco-santos Dec 9, 2020

Gozala Nov 16, 2020

vasco-santos commented Nov 25, 2020 •

edited

Loading

vasco-santos commented Dec 3, 2020 •

edited

Loading

Gozala left a comment

Gozala Dec 9, 2020

Gozala Dec 9, 2020

vasco-santos Dec 9, 2020 •

edited

Loading

Gozala Dec 9, 2020

vasco-santos Dec 9, 2020

Gozala Dec 10, 2020

Gozala Dec 9, 2020

vasco-santos Dec 9, 2020

Gozala Dec 9, 2020

vasco-santos Dec 10, 2020

Gozala commented Dec 9, 2020

vasco-santos commented Dec 9, 2020

Gozala left a comment

Gozala Dec 9, 2020

Gozala Dec 9, 2020

chore: add typedefs #802

chore: add typedefs #802

Conversation

vasco-santos commented Nov 16, 2020 • edited Loading

jacobheun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Nov 16, 2020

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Nov 25, 2020 • edited Loading

vasco-santos commented Dec 3, 2020 • edited Loading

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala commented Dec 9, 2020

vasco-santos commented Dec 9, 2020

Gozala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos commented Nov 16, 2020 •

edited

Loading

vasco-santos commented Nov 25, 2020 •

edited

Loading

vasco-santos commented Dec 3, 2020 •

edited

Loading

vasco-santos Dec 9, 2020 •

edited

Loading