chore: update types #87

vasco-santos · 2021-03-24T15:20:17Z

This PR is a follow up PR for types support. It includes:

aegir update to latest, with new types problems fixed and ts files renamed to d.ts
pubsub with options types available
typedefs for remaining interfaces: discovery and routing

vasco-santos · 2021-03-25T14:18:50Z

src/topology/index.js

@@ -62,6 +62,9 @@ class Topology {
    return Boolean(other && other[topologySymbol])
  }

+  /**
+   * @param {any} registrar


We would need to create a circular dependency to have registrar types in the interface. We can change this interaction, but I feel we should not spend a lof of time reinventing this interaction at the moment.

hugomrdias · 2021-04-06T12:53:52Z

src/peer-routing/types.d.ts

+import PeerId from 'peer-id'
+import Multiaddr from 'multiaddr'
+
+declare class PeerRouting {


its not exported ?

I was doing a named import on libp2p, but it makes sense to export this by default. I changed it here and in the other relevant types files

src/peer-discovery/tests/index.js

rvagg · 2021-04-07T09:38:33Z

package.json

@@ -47,39 +47,43 @@
  },
  "homepage": "https://github.com/libp2p/js-interfaces#readme",
  "dependencies": {
-    "@types/bl": "^2.1.0",
+    "@types/bl": "4.1.0",


has this lost the ^ for a reason?

good catch, mistyped! I will PR this shortly

rvagg · 2021-04-07T09:50:31Z

src/transport/types.d.ts

@@ -62,7 +63,7 @@ export type MultiaddrConnectionTimeline = {

 export type MultiaddrConnection = {
  sink: Sink;
-  source: () => AsyncIterable<Uint8Array>;
+  source: AsyncIterable<Uint8Array | BufferList>;


What are the use-cases for returning a plain BufferList, are we doing this as an optimisation to avoid buffer concatenation? Do our code paths actually take advantage of this or do we end up concatenating anyway?

We use BufferList because some modules we use (example: https://github.com/bustle/streaming-iterables) also support them and it is valid from users stand point, when they create libp2p protocols. Anyway, we don't use this internally anyway for optimisation and we end up concatenating as you mention.
This is something that we can look into

rvagg · 2021-04-07T09:51:40Z

src/transport/types.d.ts

@@ -1,3 +1,4 @@
+import BufferList from 'bl'


So when you import 'bl' but have @types/bl then it automagically pulls those definitions in rather than looking for the real bl (which isn't a dependency)?

yeah, if you are in typescript it will look into @types namespace. You can also do this in jsdoc for specifying typedefs with import:

@typedef {import('bl')} BufferList

rvagg · 2021-04-07T09:53:26Z

package.json

  },
  "devDependencies": {
-    "aegir": "^29.2.0",
+    "@types/debug": "^4.1.5",


I'm assuming this is a devdep because it's an internal concern and we only want it to typecheck internal consistency and don't expose debug to users in any way form here. Is that correct?

yes, that is correct!

rvagg · 2021-04-07T09:55:21Z

src/content-routing/types.d.ts

+declare class ContentRouting {
+  constructor (options: Object);
+  provide (cid: CID): Promise<void>;
+  findProviders (cid: CID, options: Object): AsyncIterable<{ id: PeerId, multiaddrs: Multiaddr[] }>;


maybe better to define a Provider here rather than an anonymous type for {id, multiaddrs}?

This is more a representation of a peer rather then a Provider. As you pointed below, findPeer also has the same. We have been discussing about potentially change the API return type of this in #85

We will probably look back into this

rvagg · 2021-04-07T09:56:24Z

src/peer-routing/types.d.ts

+
+declare class PeerRouting {
+  constructor (options?: Object);
+  findPeer (peerId: PeerId, options?: Object): Promise<{ id: PeerId, multiaddrs: Multiaddr[] }>;


the {id,multiaddrs} appears twice here again, further suggests it should be it's own Provider type

rvagg · 2021-04-07T09:59:09Z

src/pubsub/index.js

@@ -589,7 +595,7 @@ class PubsubBaseProtocol extends EventEmitter {
    for (const topic of message.topicIDs) {
      const validatorFn = this.topicValidators.get(topic)
      if (!validatorFn) {
-        continue
+        continue // eslint-disable-line 


you could just flip the logic here to if (validatorFn) { await validatorFn(..) } to avoid the disable

I will also update this in the follow up PR

rvagg · 2021-04-07T09:59:47Z

src/pubsub/message/index.js

-exports.RPC = RPC
-exports.Message = RPC.Message
-exports.SubOpts = RPC.SubOpts
+module.exports = {


out of interest: was this change forced by the type checking?

By the linter, but I think this can cause problems with inferred types and some bundlers. @hugomrdias has the bigger picture here on why we should avoid this

vasco-santos force-pushed the chore/update-types branch 6 times, most recently from 3d88eae to 17f4031 Compare March 24, 2021 17:28

chore: update types

97bebee

vasco-santos force-pushed the chore/update-types branch 8 times, most recently from fc35ea6 to 7a861c4 Compare March 25, 2021 14:01

vasco-santos commented Mar 25, 2021

View reviewed changes

vasco-santos force-pushed the chore/update-types branch from 7a861c4 to 12bd9eb Compare March 25, 2021 14:20

vasco-santos mentioned this pull request Mar 26, 2021

chore: config typescript libp2p/js-libp2p#904

Merged

6 tasks

vasco-santos force-pushed the chore/update-types branch 3 times, most recently from 09f867b to ba4b7a3 Compare March 26, 2021 21:31

chore: add routing and discovery interfaces

c34a1fb

vasco-santos force-pushed the chore/update-types branch from ba4b7a3 to c34a1fb Compare March 28, 2021 14:11

fix: sink returntype

d10b3e6

vasco-santos force-pushed the chore/update-types branch from dc8a342 to d10b3e6 Compare March 29, 2021 12:06

vasco-santos marked this pull request as ready for review March 29, 2021 13:28

vasco-santos requested review from hugomrdias and jacobheun March 29, 2021 16:36

hugomrdias suggested changes Apr 6, 2021

View reviewed changes

vasco-santos force-pushed the chore/update-types branch from d1de9aa to 5bab05b Compare April 6, 2021 15:34

hugomrdias approved these changes Apr 6, 2021

View reviewed changes

vasco-santos force-pushed the chore/update-types branch 2 times, most recently from 376b50c to a974f8f Compare April 6, 2021 20:25

chore: address review

3b99d69

vasco-santos force-pushed the chore/update-types branch from a974f8f to 3b99d69 Compare April 6, 2021 20:40

vasco-santos merged commit 64a478d into master Apr 7, 2021

vasco-santos deleted the chore/update-types branch April 7, 2021 07:39

rvagg reviewed Apr 7, 2021

View reviewed changes

vasco-santos mentioned this pull request Apr 7, 2021

chore: update pubsub interface, multiaddr and remove protons #89

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: update types #87

chore: update types #87

vasco-santos commented Mar 24, 2021 •

edited

Loading

vasco-santos Mar 25, 2021

hugomrdias Apr 6, 2021

vasco-santos Apr 6, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

chore: update types #87

chore: update types #87

Conversation

vasco-santos commented Mar 24, 2021 • 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

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 Mar 24, 2021 •

edited

Loading