feat: add types #74

vasco-santos · 2020-12-01T21:59:57Z

This PR adds typedef improvements for the already available modules, as well as typescript files for the interfaces needed in libp2p core per libp2p/js-libp2p#802

Aegir updated to latest version to support types generation
- resulting linting issues fixed
Types for interfaces: Transport, Muxer and Crypto
- should probably look into uniforming the language on the types and Readme
Switched from travis to Github actions

On top of @Gozala 's #73 PR

Needs:

type declaration files removed (currently in place to be used by libp2p PR)

BREAKING CHANGE: Some types from connection, pubsub and topology are now fixed. Record is not a class anymore, but a typescript interface that should be implemented

Gozala

@vasco-santos I did a pass here, but I think I have a more general feedback that is not pinned to any specific line:

aegir bulid will generate types in dist so no .d.ts files are necessary (I did pushed a commit here that removes them, but feel free to revert it if you disagree).
types in dist need to be mapped in package.json (I have created Feat/add types 2 #75 that does it).
type check / build is failing with this tree (I have created Feat/add types 2 #75 that addresses remaining issues)
I think we need to distinguish between interfaces (interface { ...}) and classes that implement those. And users of those implementations should code against interfaces not implementation classes. That enables anyone to do a different implementation of the interface, ensure with type checker compatibility and use it as drop-in replacement.
I think declare class-es must go, if they are necessary they should be interfaces.
There are some incompatibilities regarding MuxedStream and DuplexIterableStreams (I had to suppress some mismatches in Feat/add types 2 #75). I think we should resolve those. I don't have a complete enough understanding in this area to provide specific suggestions, but would love to have session so I can better understand it and also help resolve this.
There are bunch of places where instance fields are null|T but in code used as T (I have suppressed those with @ts-ignore in Feat/add types 2 #75, but I think we need a followup pull to resolve them)

package.json

src/crypto/types.ts

src/record/index.js

src/pubsub/index.js

src/transport/types.ts

tsconfig.json

src/transport/types.ts

src/pubsub/index.js

src/pubsub/peer-streams.js

src/pubsub/index.js

src/crypto/types.ts

src/connection/connection.js

vasco-santos · 2020-12-02T12:51:36Z

Thanks for the reviews. On @Gozala feedback:

aegir bulid will generate types in dist so no .d.ts files are necessary (I did pushed a commit here that removes them, but feel free to revert it if you disagree).
types in dist need to be mapped in package.json (I have created #75 that does it).

Yes, as mentioned on the PR desc, I kept them to test it with libp2p/js-libp2p#802
Unfortunately this will be a limitation for testing stuff across multiple PR dependencies.

I think we need to distinguish between interfaces (interface { ...}) and classes that implement those. And users of those implementations should code against interfaces not implementation classes. That enables anyone to do a different implementation of the interface, ensure with type checker compatibility and use it as drop-in replacement.
I think declare class-es must go, if they are necessary they should be interfaces.

I added them to go around the type checker error of the type not being constructable. Anyway, this did not fix the issue, so I will remove them.

There are some incompatibilities regarding MuxedStream and DuplexIterableStreams (I had to suppress some mismatches in #75). I think we should resolve those. I don't have a complete enough understanding in this area to provide specific suggestions, but would love to have session so I can better understand it and also help resolve this.

Yeah, I think they should all be MuxedStream, if you are talking about Pubsub. I will change them

There are bunch of places where instance fields are null|T but in code used as T (I have suppressed those with @ts-ignore in #75, but I think we need a followup pull to resolve them)

I will have a look on these

Gozala

Add few nits, but looks good to me to go with or without them.

src/pubsub/message/sign.js

Gozala · 2020-12-02T21:58:37Z

src/pubsub/index.js

+ * @typedef {import('./message').Message} RPCMessage
+ * @typedef {import('./signature-policy').SignaturePolicyType} SignaturePolicyType
+ */
+
 /**
 * @typedef {Object} InMessage
 * @property {string} [from]


Following code seems to suggest that from isn't optional here
https://github.com/libp2p/js-libp2p-interfaces/pull/74/files#diff-2398b30d2245a825770bfde7b95fd0b65f3ce6435c3f8cd87a632f583a5b2addR66

If so can we turn it into non optional so type checker can report the error instead of leaking it into runtime ?

It is really dependent on the signature policy. Have a look into the validate function. It will be undefined or should exist according to that policy being used.
The same for other places like the verifySignature function, which validate only calls if it has a signature. I will add an error there to avoid potential future usages of the function though

src/pubsub/peer-streams.js

Gozala · 2020-12-02T22:04:27Z

src/pubsub/peer-streams.js

@@ -138,26 +140,26 @@ class PeerStreams extends EventEmitter {
  /**
   * Attach a raw outbound stream and setup a write stream
   *
-   * @param {Stream} stream
+   * @param {MuxedStream} stream
   * @returns {Promise<void>}
   */
  async attachOutboundStream (stream) {
    // If an outbound stream already exists,
    // gently close it
    const _prevStream = this.outboundStream


In that case this line is no longer needed

Suggested change

const _prevStream = this.outboundStream

We need it for the check in the end of the function to only emit stream:outbound if the connection is new. We should only emit it when the outbound stream is ready, so this should be in the end, which means we need that auxiliary constant

Gozala · 2020-12-02T22:08:51Z

src/record/index.js

@@ -7,25 +7,31 @@ const errcode = require('err-code')
 */
 class Record {


If thing does not need to be a class it's best to define it as interface / type because it could be substituted with other implementation or even just plain object that conforms the interface. Classes should really just be a one possible implementation of it. Current version implies that TS will error anywhere record instanceof Record isn't true.

Gozala · 2020-12-02T22:16:51Z

src/topology/multicodec-topology.js

+   * Checks if the given value is a `MulticodecTopology` instance.
+   *
+   * @param {any} other
+   * @returns {other is MulticodecTopology}


https://www.typescriptlang.org/docs/handbook/advanced-types.html#using-type-predicates

src/transport/types.ts

Gozala · 2020-12-02T22:22:26Z

So, my suggestion here is that we punt on this until we get the types into the transports codebase. By that time, we can really see the advantages/limitations and decide if we should add here a second interface for the constructor. What do you thing?

Seeing the prototype field and new in the interface definition, I do really think it would be an easier change to do now than deferring it to a future. It's going to be a couple of lines change but avoid having to propagate changes in the future. I'll provide specific comment in the pull.

Either way it's just a suggestion and it's your decision to make.

Gozala · 2020-12-02T22:42:56Z

Ok if I take the current typedefs for transports and try to implement it like this:

type TCPDial = { signal?: AbortSignal }
export class TCPTransport implements Transport<TCPDial> {
//            ^^ Class 'TCPTransport' incorrectly implements interface 'Transport<TCPDial>'.
  constructor(upgrader: Upgrader, ...others: any) {
    return this
  }
  /**
   * Dial a given multiaddr.
   */
  dial(ma: Multiaddr, options?: TCPDial): Promise<Connection> {
    throw 1
  }
  /**
   * Create transport listeners.
   */
  createListener(options: any, handler: (connection:Connection) => void): Listener {
    throw 2
  }
  /**
   * Takes a list of `Multiaddr`s and returns only valid addresses for the transport
   */
  filter(multiaddrs: Multiaddr[]): Multiaddr[] {
    return multiaddrs
  }
}

P.S.: It complains about the prototype field but if you remove prototype from interface it will still complain about new.

Now consider what I was suggesting instead:

type TCPDial = { signal?: AbortSignal }
export class TCPTransport implements Transport<TCPDial> {
  constructor(_upgrader: Upgrader, ..._others: any) {
  }
  /**
   * Dial a given multiaddr.
   */
  dial(_ma: Multiaddr, _options?: TCPDial): Promise<Connection> {
    throw 1
  }
  /**
   * Create transport listeners.
   */
  createListener(_options: any, _handler: (connection:Connection) => void): Listener {
    throw 2
  }
  /**
   * Takes a list of `Multiaddr`s and returns only valid addresses for the transport
   */
  filter(multiaddrs: Multiaddr[]): Multiaddr[] {
    return multiaddrs
  }
}

export const dial = <T extends { signal: AbortSignal }>
  (NetworkTransport: TransportFactory<T>, upgrader: Upgrader, ma:Multiaddr, options:T):Transport<T> => {
  const transport = new NetworkTransport(upgrader)
  transport.dial(ma, options)
  return transport
}

Unlike above example It allows implementation of the transport to claim that it implements the Transport and type checker will make sure of it and all was necessary is to separate concerns by splitting interface into two:

interface TransportFactory <DialOptions extends { signal?: AbortSignal }> {
  new (upgrader: Upgrader, ...others: any): Transport<DialOptions>;
}

/**
 * A libp2p transport is understood as something that offers a dial and listen interface to establish connections.
 */
export interface Transport <DialOptions extends { signal?: AbortSignal }> {
  /**
   * Dial a given multiaddr.
   */
  dial(ma: Multiaddr, options?: DialOptions): Promise<Connection>;
  /**
   * Create transport listeners.
   */
  createListener(options: any, handler: (connection:Connection) => void): Listener;
  /**
   * Takes a list of `Multiaddr`s and returns only valid addresses for the transport
   */
  filter(multiaddrs: Multiaddr[]): Multiaddr[];
}

Gozala · 2020-12-02T22:52:16Z

src/transport/types.ts

+  /**
+   * Create transport listeners.
+   */
+  createListener(options: any, handler: (Connection) => void): Listener;


looks like options here should be also made into generic rather than any.

I can change it to unknown, but really not much more.
Taking a look at https://github.com/libp2p/js-libp2p-interfaces/tree/master/src/transport#create-a-listener there is no interface options for the listener. Each transport will have their own.

But that also the whole point of generics to define a generic transport interface so that specific ones can specify concrete types that correspond to them just like it was in the case of dial:

interface Tansport<DialOptions extends { signal: AbortSignal }, ListenerOption> { dial(ma: Multiaddr, options?: DialOptions): Promise<Connection>; createListener(options: ListenerOptions, handler: (Connection) => void): Listener; filter(multiaddrs: Multiaddr[]): Multiaddr[]; } class TCPTransport implements Transport<TCPDialOptions, TCPListenerOptions> { dial(ma: Multiaddr, options?: DialOptions): Promise<Connection> { // ... } createListener(options: TCPListenerOptions, handler: (connection: Connection) => void): Listener { // ... } // ... } type TCPDialOptions = { // ... } type TCPListenerOptions = //...

You could even go step further and make Transport have a default type e.g.:

interface Transport <DialOptions extends AbortOptions = AbortOptions, ListenerOptions = void> { } // Will be a shortuct for implemnets TCPTransport<AbortOptions, void> class TCPTransport implements Transport { }

src/transport/types.ts

hugomrdias · 2020-12-03T10:15:24Z

Ts check is just commented out in the config because it's fail if a repo doesn't have the tsconfig (looking at you @Gozala this could be better 🙏🏼)
Regarding the transport issue I don't really understand the dial suggestion but seems that at least the prototype and new should go into a separate type *Factory or *Construtor something like that

…factory interface and minor pubsub fixes

vasco-santos · 2020-12-03T10:27:08Z

So, I did a new iteration on this with the latest feedback from @Gozala 👍 :

Record is now just an interface
Transport and StreamMuxer now have a Factory interface for the constructor
Pubsub minor fixes

This should now be ready to go.

.github/workflows/main.yml

Co-authored-by: Hugo Dias <[email protected]>

Gozala · 2020-12-03T22:02:29Z

Ts check is just commented out in the config because it's fail if a repo doesn't have the tsconfig (looking at you @Gozala this could be better 🙏🏼)

There is tsconfig.json so can't we enable it ? I'm sorry but I really don't want to deal with repos that do not have tsconfig.json because it needs to report same errors as running tsc locally. If we don't want to have a tsconfig.json in repo we could just run aegir ts --preset config > tsconfig.json prior to running the checker.

Gozala · 2020-12-03T22:08:45Z

Regarding the transport issue I don't really understand the dial suggestion but seems that at least the prototype and new should go into a separate type *Factory or *Construtor something like that

Have you seen this comment #74 (comment) it contains links to the TS playground illustrating issues. IMO whole point of the interface is to be able to tell in the class implementation that it implementns some interface. Current version is not implementable (or more like it's not doing what I think assumbtion is about it). There is also a second link that illustrates how it could be split into two interfaces such that class can say it implements Transport and the other TrasportFactory (or whatever you want to call it) that can be used in places that need a thing that instantiates a Transport.

P.S.: prototype field isn't required by either interface.

vasco-santos · 2020-12-10T12:44:34Z

Per libp2p/js-libp2p#802 feedback, I added two new minor commits to fix review issues

Gozala added 8 commits November 27, 2020 12:45

fix: typedefs for MulticodecTopology

c1cb68e

feat: remove is-class that broke typings

cba6394

fix: add type annotations to improve innference

4e25c6e

chore: generate fixed types

7ccaf27

fix: missing type imports

f2d6a76

fix: tsconfig to generate optionals correctly

3143efd

fix: lint issues by disabling valid-jsdoc rules

b7f1072

fix: optional fields

7fd26cf

vasco-santos force-pushed the feat/add-types branch 2 times, most recently from e37aec0 to f2ab5b9 Compare December 1, 2020 22:10

feat: add types

6a95834

vasco-santos force-pushed the feat/add-types branch from f2ab5b9 to 6a95834 Compare December 1, 2020 22:12

This was referenced Dec 1, 2020

chore: add typedefs libp2p/js-libp2p#802

Merged

chore: add duplex iterable type to connection #71

Closed

vasco-santos requested review from Gozala, hugomrdias and jacobheun December 1, 2020 22:19

chore: remove generated types from vsc

bac57b0

Gozala mentioned this pull request Dec 2, 2020

Feat/add types 2 #75

Merged

Gozala suggested changes Dec 2, 2020

View reviewed changes

fix: type incompatibilities (#75)

67a5f51

hugomrdias suggested changes Dec 2, 2020

View reviewed changes

vasco-santos added 2 commits December 2, 2020 16:04

chore: address review

6f09061

chore: use github actions and remove travis

8fdc05d

vasco-santos force-pushed the feat/add-types branch from 6f53c32 to 8fdc05d Compare December 2, 2020 15:12

chore: add interface constructors

0801fc3

vasco-santos force-pushed the feat/add-types branch from 30fe22d to 0801fc3 Compare December 2, 2020 15:35

vasco-santos requested review from Gozala and hugomrdias December 2, 2020 15:46

Gozala approved these changes Dec 2, 2020

View reviewed changes

Gozala reviewed Dec 2, 2020

View reviewed changes

src/transport/types.ts Outdated Show resolved Hide resolved

src/transport/types.ts Outdated Show resolved Hide resolved

Gozala reviewed Dec 2, 2020

View reviewed changes

src/transport/types.ts Outdated Show resolved Hide resolved

vasco-santos added 2 commits December 3, 2020 11:21

chore: record interface instead of class, transport and stream muxer …

7597875

…factory interface and minor pubsub fixes

chore: remove record class file

56e606e

vasco-santos requested review from hugomrdias and removed request for hugomrdias December 3, 2020 10:27

hugomrdias approved these changes Dec 3, 2020

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

chore: enable ts error reporter

0b4b09f

Co-authored-by: Hugo Dias <[email protected]>

Gozala approved these changes Dec 3, 2020

View reviewed changes

chore: add listener options

e73646c

vasco-santos mentioned this pull request Dec 9, 2020

Support for Typescript users libp2p/js-libp2p#659

Closed

hugomrdias approved these changes Dec 9, 2020

View reviewed changes

vasco-santos added 2 commits December 10, 2020 11:15

chore: update interface record per libp2p pr feedback

a99b958

chore: make metadata optional on addStream

a24fafd

vasco-santos mentioned this pull request Dec 10, 2020

Chore/add typedefs peerstore book template 2 libp2p/js-libp2p#831

Closed

vasco-santos merged commit e2419ea into master Dec 10, 2020

vasco-santos deleted the feat/add-types branch December 10, 2020 13:03

This was referenced Dec 10, 2020

fix: typedefs for MulticodecTopology #73

Closed

chore: remove old pre release script #76

Merged

fix: pubsub publish message should be uint8array #77

Merged

vasco-santos restored the feat/add-types branch April 6, 2021 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add types #74

feat: add types #74

vasco-santos commented Dec 1, 2020 •

edited

Loading

Gozala left a comment

vasco-santos commented Dec 2, 2020

Gozala left a comment

Gozala Dec 2, 2020

vasco-santos Dec 3, 2020 •

edited

Loading

Gozala Dec 2, 2020

vasco-santos Dec 3, 2020

Gozala Dec 2, 2020

Gozala Dec 2, 2020

Gozala commented Dec 2, 2020

Gozala commented Dec 2, 2020 •

edited

Loading

Gozala Dec 2, 2020

vasco-santos Dec 3, 2020

Gozala Dec 3, 2020 •

edited

Loading

hugomrdias commented Dec 3, 2020

vasco-santos commented Dec 3, 2020

Gozala commented Dec 3, 2020

Gozala commented Dec 3, 2020

vasco-santos commented Dec 10, 2020

		@@ -7,25 +7,31 @@ const errcode = require('err-code')
		*/
		class Record {

feat: add types #74

feat: add types #74

Conversation

vasco-santos commented Dec 1, 2020 • edited Loading

Gozala left a comment

Choose a reason for hiding this comment

vasco-santos commented Dec 2, 2020

Gozala left a comment

Choose a reason for hiding this comment

Gozala Dec 2, 2020

Choose a reason for hiding this comment

vasco-santos Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

Gozala Dec 2, 2020

Choose a reason for hiding this comment

vasco-santos Dec 3, 2020

Choose a reason for hiding this comment

Gozala Dec 2, 2020

Choose a reason for hiding this comment

Gozala Dec 2, 2020

Choose a reason for hiding this comment

Gozala commented Dec 2, 2020

Gozala commented Dec 2, 2020 • edited Loading

Gozala Dec 2, 2020

Choose a reason for hiding this comment

vasco-santos Dec 3, 2020

Choose a reason for hiding this comment

Gozala Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

hugomrdias commented Dec 3, 2020

vasco-santos commented Dec 3, 2020

Gozala commented Dec 3, 2020

Gozala commented Dec 3, 2020

vasco-santos commented Dec 10, 2020

vasco-santos commented Dec 1, 2020 •

edited

Loading

vasco-santos Dec 3, 2020 •

edited

Loading

Gozala commented Dec 2, 2020 •

edited

Loading

Gozala Dec 3, 2020 •

edited

Loading