Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor fixes (types and validation) #168

Closed
wants to merge 4 commits into from

Conversation

robertkiel
Copy link

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
Member

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
Author

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
Member

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
Author

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
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants