feat: add types by hugomrdias · Pull Request #189 · multiformats/js-multiaddr

hugomrdias · 2021-03-31T15:34:43Z

This PR adds types, updates aegir and others deps.

BREAKING CHANGES:

entry point uses named exports

// before

const multiaddr = require('multiaddr')
multiaddr.resolvers
multiaddr.protocols

// after

const {multiaddr , Multiaddr, protocols, resolvers} = = require('multiaddr')
Multiaddr.resolvers
Multiaddr.protocols

Multiaddr is a normal class now
toOptions output changed to match node

// before
multiaddr('/ip4/127.0.0.1/tcp/4001').toOptions()
{ family: 'ipv4', host: '127.0.0.1', transport: 'tcp', port: 4001 }

// after
new Multiaddr('/ip4/127.0.0.1/tcp/4001').toOptions()
{ family: 4, host: '127.0.0.1', transport: 'tcp', port: 4001 }

fromNodeAddress and nodeAddress inputs/outputs now match

// before the family type was not the same between them
multiaddr('/ip4/127.0.0.1/tcp/4001').nodeAddress()
{family: 4, address: '127.0.0.1', port: '4001'}

multiaddr.fromNodeAddress({family: 'IPv4', address: '127.0.0.1', port: '4001'}, 'tcp')
<Multiaddr 047f000001060fa1 - /ip4/127.0.0.1/tcp/4001>

// after 
new Multiaddr('/ip4/127.0.0.1/tcp/4001').nodeAddress()
{family: 4, address: '127.0.0.1', port: 4001}

Multiaddr.fromNodeAddress({family: 4, address: '127.0.0.1', port: '4001'}, 'tcp')
<Multiaddr 047f000001060fa1 - /ip4/127.0.0.1/tcp/4001>

closes #160
closes #159

This PR also updates aegir and others deps.

vasco-santos

Almost done, left a few comments for final details and we need to decide if we want to do a breaking change or not here, in the toOptions return type

Also, probably worth flag that we should add to the release notes a notice for deprecation of the factory to already give a heads up to users

vasco-santos · 2021-04-06T12:39:04Z

README.md

-> const multiaddr = require('multiaddr')
+> const { multiaddr } = require('multiaddr')

 > const addr = multiaddr("/ip4/127.0.0.1/udp/1234")


Suggested change

> const addr = multiaddr("/ip4/127.0.0.1/udp/1234")

> const addr = new Multiaddr("/ip4/127.0.0.1/udp/1234")

there a factory method exported

yes, but all the other code examples in jsdocs are with the constructor, why having these different? I also thought we were going to migrate into using the class instance in the future

the factory method returns an instance, its just an helper i will add an example of both

vasco-santos · 2021-04-06T12:39:26Z

README.md

 $ node

-> const multiaddr = require('multiaddr')
+> const { multiaddr } = require('multiaddr')


Suggested change

> const { multiaddr } = require('multiaddr')

> const { Multiaddr } = require('multiaddr')

theres a factory method exported

yes, but all the other code examples in jsdocs are with the constructor, why having these different? I also thought we were going to migrate into using the class instance in the future

package.json

src/codec.js

src/resolvers/dns.js

src/resolvers/dns.browser.js

src/resolvers/index.js

vasco-santos · 2021-04-06T13:06:54Z

src/index.js

+    /** @type {MultiaddrObject} */
+    const opts = {}
+    const parsed = this.toString().split('/')
+    opts.family = parsed[1] === 'ip4' ? 4 : 6


This will mean a breaking change

can you add a breaking change message to the commit?

I am thinking if we really want to make a breaking change just for this change and removing the resolve static function. This will mean we need to update all the modules. What do you think?

its a breaking change already because the exports changed right ?

src/index.js

vasco-santos · 2021-04-07T08:41:32Z

@hugomrdias can you look into the CI?

rvagg

sgtm, for what that's worth

hugomrdias · 2021-04-07T10:32:56Z

update the readme usage example with the factory function, remove the resolve test and update the PR description with detailed examples for the breaking changes

vasco-santos

LGTM

The webworker tests have an issue though

vasco-santos · 2021-04-07T11:13:34Z

We have an issue on firefox worker still. Can you check it?

rvagg · 2021-04-08T07:19:40Z

Is there any chance the Firefox problem was related to the one @achingbrain had with js-ipfs which forced him to disable it? I think it was with webworkers running twice there but the output doesn't seem to suggest the same here (although there isn't much useful output, just a timeout).

hugomrdias · 2021-04-08T07:23:16Z

Is there any chance the Firefox problem was related to the one @achingbrain had with js-ipfs which forced him to disable it? I think it was with webworkers running twice there but the output doesn't seem to suggest the same here (although there isn't much useful output, just a timeout).

No, i already fixed this just need to release pw-test. Double output should still happen though.

vasco-santos

LGTM

feat: add types

354c5c0

This PR also updates aegir and others deps.

hugomrdias self-assigned this Mar 31, 2021

fix: increase size

86e44e2

vasco-santos self-requested a review April 6, 2021 11:31

vasco-santos requested changes Apr 6, 2021

View reviewed changes

fix: feedback

1e150b2

hugomrdias requested a review from vasco-santos April 6, 2021 16:44

fix: remove resolve and update aegir

d18f06b

rvagg approved these changes Apr 7, 2021

View reviewed changes

fix: remove resolve test and improve readme usage

77c22b5

vasco-santos reviewed Apr 7, 2021

View reviewed changes

hugomrdias added 2 commits April 7, 2021 11:41

chore: fix worker ci

a58de4d

chore: remove double browser test in ci

6130f82

hugomrdias requested a review from vasco-santos April 7, 2021 10:45

vasco-santos approved these changes Apr 8, 2021

View reviewed changes

vasco-santos merged commit 7d284e4 into master Apr 8, 2021

vasco-santos deleted the feat/add-types-2 branch April 8, 2021 12:32

	> const addr = multiaddr("/ip4/127.0.0.1/udp/1234")
	> const addr = new Multiaddr("/ip4/127.0.0.1/udp/1234")

	> const { multiaddr } = require('multiaddr')
	> const { Multiaddr } = require('multiaddr')

Conversation

hugomrdias commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasco-santos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasco-santos commented Apr 7, 2021

Uh oh!

rvagg left a comment

Choose a reason for hiding this comment

Uh oh!

hugomrdias commented Apr 7, 2021

Uh oh!

vasco-santos left a comment

Choose a reason for hiding this comment

Uh oh!

vasco-santos commented Apr 7, 2021

Uh oh!

rvagg commented Apr 8, 2021

Uh oh!

hugomrdias commented Apr 8, 2021

Uh oh!

vasco-santos left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hugomrdias commented Mar 31, 2021 •

edited

Loading

vasco-santos left a comment •

edited

Loading