chore: config typescript #904

vasco-santos · 2021-03-26T09:47:08Z

This PR aims to:

update dependencies
switch protons to protobufjs
- a bunch of files were automatically generated by this, which is a significant part of the PR changes
  - "src/circuit/protocol/index.js" and "...d.ts"
  - "src/insecure/proto.js" and "...d.ts"
  - "src/peer-store/persistent/pb/address-book.js" and "...d.ts"
  - "src/peer-store/persistent/pb/proto-book.js" and "...d.ts"
  - "src/record/peer-record/peer-record.js" and "...d.ts"
  - "src/record/envelope/envelope.js" and "...d.ts"
update aegir to latest version
- fix types per no explicitly any allowed
- added esbuild
guarantee all the possible libp2p configurations documented on GettingStarted.md and Configuration.md completely work with typescript projects
- added test ts project
some bugs fixed, comments inline in review
restructure github actions into a new workflow for examples

There are internal breaking changes in the Dialer and Keychain property names to match what comes from the external config

Needs:

Unblocks:

Closes: #891
Solves some of the items from #830 (DHT, Config and Keychain)

BREAKING CHANGES:

top level types were updated, multiaddr@9.0.0 is used, dialer and keychain internal property names changed and connectionManager minPeers is not supported anymore

vasco-santos · 2021-03-26T10:05:34Z

.github/workflows/main.yml

@@ -12,10 +12,10 @@ jobs:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
-    - run: yarn
-    - run: yarn lint
+    - run: npm install


Because yarn does not run prepare in the dependencies, libp2p-interfaces branch does not have types

vasco-santos · 2021-03-26T10:07:29Z

src/dialer/index.js

-    concurrency = MAX_PARALLEL_DIALS,
-    timeout = DIAL_TIMEOUT,
-    perPeerLimit = MAX_PER_PEER_DIALS,
+    maxParallelDials = MAX_PARALLEL_DIALS,


This is an internal breaking change. No one should be creating a dialer from here though. If we don't want to change this, we can create a separate typedef for core options and keep things as they were. However, keeping different names for the same things does not seem the best call

"No one should be" famous last words, how much confidence do you have in this?

Does not make sense to require the Dialer and use it as a specific piece. However, I agree and it will be safe to just ship a breaking change at this point

If you're adding types it's good to ship as a major anyway as there will almost certainly be some disruption.

vasco-santos · 2021-03-26T10:12:42Z

src/index.js

+ * @property {number} [interval = 300e3]
+ * @property {number} [timeout = 10e3]
+ *
+ * @typedef {Object} DhtOptions


This should probably go away when we work on the configuration improvements project. We currently also specify the DHT options in https://github.com/libp2p/js-libp2p/blob/master/src/config.js#L52 , but in theory we should accept other DHT implementations and this might mean different configurations.

vasco-santos · 2021-03-26T10:38:53Z

src/keychain/index.js

@@ -118,8 +130,8 @@ class Keychain {
    this.opts = mergeOptions(defaultOptions, options)

    // Enforce NIST SP 800-132
-    if (this.opts.passPhrase && this.opts.passPhrase.length < 20) {
-      throw new Error('passPhrase must be least 20 characters')
+    if (this.opts.pass && this.opts.pass.length < 20) {


Same situation as in the Dialer

vasco-santos · 2021-03-29T16:46:36Z

src/connection-manager/index.js

@@ -362,7 +365,7 @@ class ConnectionManager extends EventEmitter {
   */
  _maybeDisconnectOne () {
    if (this._options.minConnections < this.connections.size) {
-      const peerValues = Array.from(this._peerValues).sort(byPeerValue)
+      const peerValues = Array.from(new Map([...this._peerValues.entries()].sort((a, b) => a[1] - b[1])))


This was a bug! this.peerValues is a map<string, number>

vasco-santos · 2021-03-29T16:52:56Z

src/registrar.js

    for (const [, topology] of this.topologies) {
-      topology.disconnect(connection.remotePeer, error)
+      topology.disconnect(connection.remotePeer)


topology.disconnect only has one parameter

examples/libp2p-in-the-browser/package.json

rvagg · 2021-04-07T10:06:02Z

examples/webrtc-direct/package.json

@@ -3,6 +3,7 @@
  "version": "0.0.1",
  "private": true,
  "description": "",
+  "main": "dist/index.html",


I'm guessing it's a parcel thing ..?

parcel thing per their new updates

rvagg · 2021-04-07T10:07:44Z

package.json

@@ -127,22 +137,24 @@
    "libp2p-delegated-peer-routing": "^0.8.0",


lots of ^ in here for <1 dependencies, seems over-optimistic about how this is going to get consumed into the future

In the future, I would like that we get confidence in the APIs to ship v1 of all the things.

My personal preference is to get to 1.0.0 ASAP (I've even set my default version to 1.0.0) because of the semver mess below 1 is just madness for signalling and ranges. The rules are basically "YOLO" which is useless for downstream users.
The problem with <0 is that your only signal for "breaking" changes is the minor version, but we've got ^ in here so there's no way to block breaking changes slipping in when we update one of these components. For <1, it's usually best to use ~ to account for the semver-minor bumps for breaking changes. Anyone using the current version of the library, even if they pin to the specific version, is going to get a newer, broken transitive dependency (like libp2p-delegated-peer-routing) when they do their next npm install because we've shipped with ^.
Alternatively, just don't treat 1.0.0 as anything special and ship it so we can use semver properly from there on.

Yes,I totally agree on this. The team has had a lot of concerns about shipping 1.0.0, but we need to have a plan to get to this point. I think that as part of sudo work we should create a plan on where should we be to land 1.0.0 on everything

Please ship v1 of things!

Alternatively, just don't treat 1.0.0 as anything special and ship it so we can use semver properly from there on.

Yes!

#680

we will need to do this refactor as part of the maintenance work and will affect most of our repos. So, this might be a good opportunity to ship v1 of libp2p modules

rvagg · 2021-04-07T10:16:21Z

src/insecure/plaintext.js

 const { Exchange, KeyType } = require('./proto')
 const protocol = '/plaintext/2.0.0'

 /**
 * @typedef {import('libp2p-interfaces/src/connection').Connection} Connection
 */

+/**
+ * @param {{ id: Uint8Array; pubkey: { Type: any, Data: Uint8Array; }; }} exchange


potential candidate for a named type?

fixed by relying on protobufjs instead of protons, which offers us all the types for protobuf definitions

rvagg · 2021-04-07T10:18:16Z

src/metrics/stats.js

@@ -82,7 +82,7 @@ class Stats extends EventEmitter {
  /**
   * Returns a clone of the internal movingAverages
   *
-   * @returns {Object}
+   * @returns {typeof Object.assign}


weird, can you explain this one?

No need for this, it can be inferred automatically. Good catch

rvagg

what's the versioning strategy going to be with this one? seems like a lot of potential to break for users - aside from the Dialer options the types might introduce breakage?

vasco-santos · 2021-04-08T13:15:36Z

what's the versioning strategy going to be with this one? seems like a lot of potential to break for users - aside from the Dialer options the types might introduce breakage?

My initial idea was to ship a patch, but with all the types changes, I feel it will be safer to release a breaking change. This will have consequences on the release chain of upcoming releases (pinned issues), but they will probably be affected either way.

vasco-santos · 2021-04-13T11:16:50Z

src/index.js

@@ -152,7 +192,9 @@ class Libp2p extends EventEmitter {
    this._discovery = new Map() // Discovery service instances/references

    // Create the Connection Manager
-    if (this._options.connectionManager.minPeers) { // Remove in 0.29
+    // @ts-ignore deprecated, needs to be removed on breaking change


Since we are doing a breaking change, I am going to remove this!

achingbrain

LGTM, just some small nits.

.github/workflows/main.yml

achingbrain · 2021-04-14T11:14:56Z

.github/workflows/main.yml

@@ -12,11 +12,11 @@ jobs:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v2
-    - run: yarn
-    - run: yarn lint
-    - uses: gozala/typescript-error-reporter-action@v1.0.8


Did this get pulled out intentionally? It is quite useful.

I changed based on: https://github.com/ipfs/aegir/blob/master/md/github-actions.md

Thought the recommendation was ts -p check

The error reporter action shows you which lines have errors in a visual form as part of the PR, the aegir command makes you dig through the console output. Both do the same thing and use the same rules but the action is a little more user friendly IMHO.

test/core/ping.node.js

src/types.d.ts

Co-authored-by: Alex Potsides <alex@achingbrain.net>

vasco-santos force-pushed the chore/config-ts branch from 141f2b3 to 170ebfb Compare March 26, 2021 10:04

vasco-santos commented Mar 26, 2021

View reviewed changes

vasco-santos force-pushed the chore/config-ts branch 4 times, most recently from 718bc3d to 09fb967 Compare March 27, 2021 12:16

chore: config typescript

cec3ce7

vasco-santos force-pushed the chore/config-ts branch from 09fb967 to 5e9c17d Compare March 29, 2021 13:28

chore: update aegir

a3a2f15

vasco-santos force-pushed the chore/config-ts branch from 5e9c17d to a3a2f15 Compare March 29, 2021 13:29

chore: increase bundle size

ab0f087

vasco-santos mentioned this pull request Mar 29, 2021

Remove libp2p-ts types ChainSafe/lodestar#2167

Closed

vasco-santos force-pushed the chore/config-ts branch 9 times, most recently from 754a7b9 to 59a40a3 Compare March 29, 2021 16:39

vasco-santos commented Mar 29, 2021

View reviewed changes

vasco-santos force-pushed the chore/config-ts branch from 59a40a3 to a18ed22 Compare March 29, 2021 17:06

chore: add ts tests

b0db319

vasco-santos force-pushed the chore/config-ts branch 3 times, most recently from 9c1d7da to 78019a1 Compare March 29, 2021 20:22

vasco-santos requested review from achingbrain and jacobheun April 6, 2021 09:52

rvagg reviewed Apr 7, 2021

View reviewed changes

examples/libp2p-in-the-browser/package.json Show resolved Hide resolved

rvagg reviewed Apr 7, 2021

View reviewed changes

chore: replace protons with protobufjs

2bff878

vasco-santos force-pushed the chore/config-ts branch from bd10429 to 2bff878 Compare April 8, 2021 13:12

rvagg mentioned this pull request Apr 10, 2021

[CoreTypes] Tracking protocol/w3dt-sudo#17

Open

77 tasks

chore: update interfaces and multiaddr

040495d

vasco-santos force-pushed the chore/config-ts branch from 58bf45e to 040495d Compare April 12, 2021 16:17

vasco-santos added 2 commits April 12, 2021 18:42

chore: add prepare script

55bfbe2

chore: identify to protobufjs instead of protons

5159d23

vasco-santos force-pushed the chore/config-ts branch from f436ef9 to 5159d23 Compare April 13, 2021 08:52

chore: decrease bundle size

e69eba6

vasco-santos commented Apr 13, 2021

View reviewed changes

chore: remove url from node globals

8e91334

vasco-santos force-pushed the chore/config-ts branch from c47f022 to 8e91334 Compare April 13, 2021 11:53

vasco-santos mentioned this pull request Apr 13, 2021

chore: update deps libp2p/js-libp2p-kad-dht#217

Merged

2 tasks

vasco-santos marked this pull request as ready for review April 13, 2021 13:05

vasco-santos mentioned this pull request Apr 14, 2021

TypeError: TextDecoder is not a constructor when handling multiaddresses in browser #909

Closed

achingbrain requested changes Apr 14, 2021

View reviewed changes

vasco-santos force-pushed the chore/config-ts branch from 5d10d7e to d206a25 Compare April 14, 2021 13:09

chore: apply suggestions from code review

98325d9

Co-authored-by: Alex Potsides <alex@achingbrain.net>

vasco-santos force-pushed the chore/config-ts branch from d206a25 to 98325d9 Compare April 14, 2021 13:22

vasco-santos requested a review from achingbrain April 14, 2021 13:52

achingbrain approved these changes Apr 15, 2021

View reviewed changes

vasco-santos merged commit 8506414 into master Apr 15, 2021

vasco-santos deleted the chore/config-ts branch April 15, 2021 07:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: config typescript #904

chore: config typescript #904

vasco-santos commented Mar 26, 2021 •

edited

Loading

vasco-santos Mar 26, 2021

vasco-santos Mar 26, 2021

rvagg Apr 7, 2021

vasco-santos Apr 13, 2021

achingbrain Apr 14, 2021

vasco-santos Mar 26, 2021

vasco-santos Mar 26, 2021

vasco-santos Mar 29, 2021

vasco-santos Mar 29, 2021

rvagg Apr 7, 2021

vasco-santos Apr 7, 2021

rvagg Apr 7, 2021

vasco-santos Apr 8, 2021

rvagg Apr 9, 2021

vasco-santos Apr 13, 2021

achingbrain Apr 14, 2021

vasco-santos Apr 14, 2021

rvagg Apr 7, 2021

vasco-santos Apr 8, 2021

rvagg Apr 7, 2021

vasco-santos Apr 13, 2021

rvagg left a comment

vasco-santos commented Apr 8, 2021

vasco-santos Apr 13, 2021

achingbrain left a comment

achingbrain Apr 14, 2021

vasco-santos Apr 14, 2021

achingbrain Apr 15, 2021

		@@ -127,22 +137,24 @@
		"libp2p-delegated-peer-routing": "^0.8.0",

chore: config typescript #904

chore: config typescript #904

Conversation

vasco-santos commented Mar 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg left a comment

Choose a reason for hiding this comment

vasco-santos commented Apr 8, 2021

Choose a reason for hiding this comment

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

edited

Loading