Skip to content

Minor fixes (types and validation)#168

Closed
robertkiel wants to merge 4 commits intomultiformats:masterfrom
robertkiel:master
Closed

Minor fixes (types and validation)#168
robertkiel wants to merge 4 commits intomultiformats:masterfrom
robertkiel:master

Conversation

@robertkiel
Copy link
Copy Markdown

@robertkiel robertkiel commented Jan 2, 2021

Fixes:

as defined in Node.JS

NodeAddress.port: number

instead of

NodeAddress.port: string

as implemented in index.js

.getPeerId(): string | null

instead of

.getPeerId(): string

proper error for .toOptions()

Multiaddr('/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').toOptions() // Error('Invalid address family')

instead of

Multiaddr('/p2p/QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC').toOptions() => {
  family: 'ip6'
  address: 'QmcgpsyWgH8Y8ajJz1Cu72KnS5uo2Aa2LpzU7kinSupNKC'
}

@vasco-santos
Copy link
Copy Markdown
Member

vasco-santos commented Jan 7, 2021

Thanks for the PR @robertkiel

We are doing a major revamp to generate type declaration files from jsdoc. This is being worked in #159, which makes this PR redundant at this point.

@robertkiel
Copy link
Copy Markdown
Author

robertkiel commented Jan 12, 2021

Thanks for the PR @robertkiel

We are doing a major revamp to generate type declaration files from jsdoc. This is being worked in #159, which makes this PR redundant at this point.

Sounds good 👍 my suggestions are more related to inconveniences resp. incompatibilities with NodeJS and IP addresses.

@vasco-santos
Copy link
Copy Markdown
Member

vasco-santos commented Jan 12, 2021

I see, so would you mind adding them on top of #159 in a new PR if they are still an issue on that PR?

@robertkiel
Copy link
Copy Markdown
Author

robertkiel commented Jan 12, 2021

Sounds good. Could you give me an estimate when how close you are? Will create a PR if necessary once you are mostly done.

@vasco-santos
Copy link
Copy Markdown
Member

vasco-santos commented Jan 13, 2021

Thanks, I hope soon. @hugomrdias can you let us know in this PR once #159 gets merged?

@hugomrdias hugomrdias mentioned this pull request Jan 13, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@robertkiel @vasco-santos