Add Typescript support #150

nazarhussain · 2021-06-14T22:09:29Z

With reference to the issue libp2p/js-libp2p#659 this PR will add typescript support to peer-id

nazarhussain · 2021-06-14T22:15:12Z

Two questions for the reviewer:

I am not sure if we really need the below code. What it's actually doing is creating isPeerId function on the class, but we have our own implementation of isPeerId so we really don't need it.

Lines 497 to 500 in 42efbb5

    
           module.exports = withIs(PeerId, { 
        
             className: 'PeerId', 
        
             symbolName: '@libp2p/js-peer-id/PeerId' 
        
           })

The implementation of get pubKey suggests it could be undefined. So if we use the right type fo get pubcKey as PublicKey | undefined it could be breaking change for users. If we force it to not undefined then type will not match the implementaiton.

js-peer-id/src/index.js

Lines 48 to 67 in e19da79

    
           get pubKey () { 
        
             if (this._pubKey) { 
        
               return this._pubKey 
        
             } 
        
             if (this._privKey) { 
        
               return this._privKey.public 
        
             } 
        
             try { 
        
               const decoded = mh.decode(this.id) 
        
               if (decoded.name === 'identity') { 
        
                 this._pubKey = cryptoKeys.unmarshalPublicKey(decoded.digest) 
        
               } 
        
             } catch (_) { 
        
               // Ignore, there is no valid public key 
        
             } 
        
             return this._pubKey

Please suggest how to handle these two cases.

vasco-santos · 2021-06-16T16:38:42Z

Thanks for reaching out @nazarhussain

class-is

yes, it should be removed. We should have something similar to https://github.com/multiformats/js-multiaddr/blob/master/src/index.js#L550 where we also had before class-is before moving with the types work

public key

yes, sadly it can also return undefined. The story here is that not all types of key types allows us to compute the public key with the peerId. And you can create a PeerId just with the id. You mention this will break the manually implemented types in https://github.com/libp2p/js-peer-id/blob/master/src/index.d.ts#L118 right?

The answer here is that we need to make breaking changes as the types do not represent the truth and undefined can happen

nazarhussain · 2021-06-17T22:01:31Z

@vasco-santos PR is ready for review. Feel free to share your feedback. Just to be sure this PR contains breaking changes in terms of types not behaviour.

vasco-santos · 2021-06-22T14:51:38Z

@nazarhussain thanks, sorry for taking long to get here. I will review this tomorrow

vasco-santos

Thanks for submitting these @nazarhussain . I left some feedback, but I think we are close here

vasco-santos · 2021-06-23T07:17:00Z

src/index.js

@@ -143,7 +223,8 @@ class PeerId {
        enumerable: false
      })
    }
-    return this._idCIDString
+
+    return this._idCIDString ? this._idCIDString : ''


there will always exist a this._idCIDString at this point, right?

If this is a typescript complaint, probably better to add a @ts-ignore and also point to the issue in typescript: microsoft/TypeScript#28694 rather than having this "impossible" possibility of returning an invalid peerId string (empty string). What do you think?

It was the issue of Non-nullable types was not familiar its different than typescript.

src/index.js

vasco-santos · 2021-06-23T09:29:02Z

src/index.js

  if (val) {
    return uint8ArrayToString(val, 'base64pad')
  }
+
+  return ''


I don't like returning things that are not valid. I think we just do in toJson:

const privKey = this.marshalPrivKey() const pubKey = this.marshalPubKey() return { id: this.toB58String(), privKey: privKey && toB64Opt(privKey), pubKey: pubKey && toB64Opt(pubKey) }

This way, this function could not accept undefined and this would return what is suppoed or throw

nazarhussain · 2021-06-24T18:28:54Z

@vasco-santos Thanks for such a detail feedback. My intention was to make minimum change possible to existing code which does not involve making changes to the tests. Hopefully it looks better now.

Currently let with these linting warning/errors.

index.js:5:1: Unexpected 'todo' comment: 'TODO: Fix missing type'. [Warning/no-warning-comments]
index.js:69:5: Expected an assignment or function call and instead saw an expression. [Error/no-unused-expressions]
index.js:274:5: Unexpected 'todo' comment: 'TODO: needs better checking'. [Warning/no-warning-comments]
index.js:462:5: Unexpected 'todo' comment: 'TODO: val id and pubDigest'. [Warning/no-warning-comments]

I am not sure how to disable warnings for such TODO comments, please suggest.

For the error case it comes because of this line in constructor.

    /**
     * @type {!string}
     */
    this._idCIDString

Linting is expecting to assign some value there, we can disable this linting error. Else ee can assign an empty string and later we can check for empty string in toString case. It will fix the issue, but will introduce a change in this test case to have "_idCIDString": "" field in a peerId which is not stringified with toString().

https://github.com/nazarhussain/js-peer-id/blob/nh-ts-support/test/peer-id.spec.js#L312

vasco-santos

Some other bits of feedback.

FYI, we discussed yesterday about creating a PeerId interface, among other interfaces, so that they are implementation agnostic allowing us upgrade implementation without coordinating these changes across the board and avoid breaking changes like this will be.

With this in mind, I think we will wait for the PeerId interface and this would become an implementation of it. The current content of the PR is still needed, but we will need to add @implements PeerIdInterface

src/index.js

nazarhussain · 2021-06-25T15:57:21Z

@vasco-santos Thanks for the information. Let me know if I can be helpful in creating that interface.

lidel · 2021-07-02T14:36:03Z

Triage note:

next step is to create the interface
- PoC would be to create interface with everything that is not prefixed with _ + factory function
- @achingbrain will provide more details if time allows 🙏

nazarhussain · 2021-07-02T14:38:22Z

@lidel @vasco-santos If you guys allow I can create the interface for PeerID in interfaces repo. What do you think?

lidel · 2021-07-30T14:22:40Z

@nazarhussain yes i believe https://github.com/libp2p/js-libp2p-interfaces is the place.
let us know if you still want to work on this or someone else can pick this topic

BigLep · 2021-08-27T14:13:38Z

@nazarhussain : just checking in - are you planing to add in the interface? Thanks.

nazarhussain · 2021-08-29T20:04:13Z

@BigLep I was not sure based on discussion, that should I contribute to the interfaces repo or not. But now I will create PR in few coming days.

BigLep · 2021-09-24T14:12:02Z

@nazarhussain: just checking in to see if you're going to make this PR or not. Thanks!

nazarhussain · 2021-09-24T14:20:23Z

Hi @BigLep sorry to get late. I got engaged with some other issues. I actually worked on it and created the interfaces. Was writing the test helpers where I left it. Will complete and open the PR this weekend. Then we can discuss on that PR to improve any thing further.

nazarhussain · 2021-09-24T19:57:27Z

@BigLep I just opened a PR libp2p/js-libp2p-interfaces#107. I will work on its feedback. As soon it's merged will refactor this PR according to it.

Creates `PeerId` interface and its compliance tests. The discussion was initiated from libp2p/js-libp2p#955 and libp2p/js-peer-id#150 (review) Co-authored-by: achingbrain <alex@achingbrain.net>

BigLep · 2021-11-12T15:31:44Z

@nazarhussain: given libp2p/js-libp2p-interfaces#107 got merged, are you ok to refactor this PR?

nazarhussain · 2021-11-12T22:26:07Z

@BigLep I was waiting for the newer release of libp2p-interfaces which includes that interface, so I can use it in this PR. As soon that is released I will refactor this PR and get it ready.

nazarhussain · 2021-12-25T02:30:30Z

The changes related libp2p-interfaces were released but some unintentional change was introduced. I had to create a PR for that libp2p/js-libp2p-interfaces#126 Now waiting for this to get merged and released.

BigLep · 2022-01-28T15:53:03Z

Closing because this is being handled by the TypeScript conversion push (being tracked in libp2p/js-libp2p#1021 ).

nazarhussain added 3 commits June 15, 2021 00:06

⬆️ Update aegir dependency

3320f08

🔧 Update tsconfig to follow other js-libp2p repos

c032f35

🎨 Update PeerId class use JSDoc ts types

42efbb5

⬇️ Remove class-is dependency

3312d11

vasco-santos suggested changes Jun 23, 2021

View reviewed changes

vasco-santos mentioned this pull request Jun 23, 2021

chore: update to new multiformats #149

Merged

nazarhussain added 2 commits June 24, 2021 16:09

💄 Fix file linting

99d100b

🎨 Fix the code for given feedback

a8813e0

nazarhussain requested a review from vasco-santos June 24, 2021 18:29

vasco-santos suggested changes Jun 25, 2021

View reviewed changes

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

src/index.js Outdated Show resolved Hide resolved

lidel added the status/blocked Unable to be worked further until needs are met label Jun 25, 2021

🎨 Update code as per feedback

3491720

lidel marked this pull request as draft July 30, 2021 14:20

BigLep added the need/author-input Needs input from the original author label Aug 27, 2021

nazarhussain mentioned this pull request Sep 24, 2021

feat: add PeerId interface and compliance tests libp2p/js-libp2p-interfaces#107

Merged

achingbrain mentioned this pull request Nov 26, 2021

chore: remove binary #160

Merged

nazarhussain mentioned this pull request Dec 25, 2021

fix: peer-id privKey and pubKey can be undefined libp2p/js-libp2p-interfaces#126

Merged

nazarhussain added 2 commits December 25, 2021 03:51

Merge branch 'master' into nh-ts-support

4185c2c

⚰️ Resolve conflicts

67ea3f1

BigLep closed this Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Typescript support #150

Add Typescript support #150

nazarhussain commented Jun 14, 2021

nazarhussain commented Jun 14, 2021

vasco-santos commented Jun 16, 2021

nazarhussain commented Jun 17, 2021

vasco-santos commented Jun 22, 2021

vasco-santos left a comment

vasco-santos Jun 23, 2021

nazarhussain Jun 24, 2021

vasco-santos Jun 23, 2021

nazarhussain commented Jun 24, 2021

vasco-santos left a comment

nazarhussain commented Jun 25, 2021

lidel commented Jul 2, 2021

nazarhussain commented Jul 2, 2021

lidel commented Jul 30, 2021 •

edited

Loading

BigLep commented Aug 27, 2021

nazarhussain commented Aug 29, 2021

BigLep commented Sep 24, 2021

nazarhussain commented Sep 24, 2021

nazarhussain commented Sep 24, 2021

BigLep commented Nov 12, 2021

nazarhussain commented Nov 12, 2021

nazarhussain commented Dec 25, 2021

BigLep commented Jan 28, 2022

Add Typescript support #150

Add Typescript support #150

Conversation

nazarhussain commented Jun 14, 2021

nazarhussain commented Jun 14, 2021

vasco-santos commented Jun 16, 2021

nazarhussain commented Jun 17, 2021

vasco-santos commented Jun 22, 2021

vasco-santos left a comment

Choose a reason for hiding this comment

vasco-santos Jun 23, 2021

Choose a reason for hiding this comment

nazarhussain Jun 24, 2021

Choose a reason for hiding this comment

vasco-santos Jun 23, 2021

Choose a reason for hiding this comment

nazarhussain commented Jun 24, 2021

vasco-santos left a comment

Choose a reason for hiding this comment

nazarhussain commented Jun 25, 2021

lidel commented Jul 2, 2021

nazarhussain commented Jul 2, 2021

lidel commented Jul 30, 2021 • edited Loading

BigLep commented Aug 27, 2021

nazarhussain commented Aug 29, 2021

BigLep commented Sep 24, 2021

nazarhussain commented Sep 24, 2021

nazarhussain commented Sep 24, 2021

BigLep commented Nov 12, 2021

nazarhussain commented Nov 12, 2021

nazarhussain commented Dec 25, 2021

BigLep commented Jan 28, 2022

lidel commented Jul 30, 2021 •

edited

Loading