fix: integrate with aegir types cmd #159

hugomrdias · 2020-11-24T17:49:32Z

This PR does the following: (review might be easier by commit)

chore: update readme to look like the other multiformats repos
fix: update package.json and tsconfig
fix: transform multiaddr into a class

BREAKING CHANGE: index.js default export no longer is both callable and
constructor. encapsulate and decapsulate only accept Multiaddr instances as
input.
fix: remove class-is and fix types in index.js
fix: add some types to protocols
fix: add some types to convert

BREAKING CHANGE: Convert.toString now always returns a string
fix: add some types to codec.js
fix: fix resolver exports
fix: remove old docs stuff
feat: use github ci

The breaking changes should be theoretically based on the current documentation and usage but we never know if people are doing something not documented.

ping this PR #168

.github/ISSUE_TEMPLATE/bug_report.md

package.json

src/codec.js

src/index.js

src/protocols-table.js

src/resolvers/dns.js

vasco-santos · 2020-11-25T14:22:38Z

src/resolvers/index.js

 /**
 * Resolver for dnsaddr addresses.
 *
- * @param {Multiaddr} addr
+ * @param {any} addr


Why not a multiaddr?

theres a TODO above explaining we can't get the Multiaddr type because of this issue microsoft/TypeScript#41672

Ah right! This is not nice as we will need to update this again after, I might wait for this to be solved before getting libp2p PR updating this merged in.
But we can move forward with this module here.

the only way to solve this now, is to do a bigger breaking change and default export the Multiaddr class directly. this would remove the ability to multiaddr('/ip..') and force new Multiaddr('')

should we do this ?

i can add a static method to the Multiaddr class called multiaddr and users would only need to change from const multiaddr = require('multiaddr) to const { multiaddr } = require('multiaddr)

I would like to have the constructor exported by default to be honest. But, I would wait on @jacobheun input before moving that way as this has a impact on a large number of modules.

yeah having the constructor exported directly makes a bunch of things easier and documentation looks a lot better.

I'm in favor of exporting the constructor, we should make sure we have clear migration docs for people. We should also add a static method for creation, this would make it easy to hot swap the two and allow us to be more picky about the constructor args. Since this is in the multiformats org it might be good to be consistent with the cid update effort, multiformats/js-multiformats#23, and call it asMultiaddr

vasco-santos

One minor detail left, and this should be good to go

src/index.js

BREAKING CHANGE: Convert.toString now always a string

BREAKING CHANGE: index.js default export no longer is both callable and constructor. encapsulate and decapsulate only accept Multiaddr instances as input.

BREAKING CHANGE: module export the Multiaddr class instead of a factory function.

hugomrdias · 2020-12-15T12:24:20Z

@jacobheun and @vasco-santos can you please review, added the migration docs and some more small tweaks as for the asMultiaddr i would like to do that in another PR.

Gozala

I made some small suggestions and some not so small. Given that this is breaking change, I think it would be good to use this opportunity to do more changes that otherwise would be a lot harder to justify:

Constructor takes address in arbitrary representation, tries to figure out what is it and then turns it into Multiaddr if it can. I think a factory functions (static method) representation kind is lot more clear both for user and implementer (as illustrated inline) and avoids unnecessary work when types are known. If it is not unreasonable I would really love to use this opportunity to do such a change. We can still have generic Multiaddr.from when representation is unknown.
Multiaddr has large number of utility methods, having equivalent static methods would be really nice to have because:

They come handy in places like addrs.map(Multiaddr.toOptions)
Moving multiaddrs across realms / threads discards prototype chains. Which means static methods continue to work, but not instance methods. Which in turn means encode / decode pipeline.

I realize above two are big asks & only bringing them so we can consider / discuss given the opportunity presented by breaking change.

Gozala · 2020-12-16T23:14:53Z

.github/workflows/main.yml

+    - run: yarn
+    - run: yarn lint
+    - run: yarn build
+    - uses: gozala/[email protected]


Suggested change

- uses: gozala/[email protected].4

- uses: gozala/[email protected].8

Gozala · 2020-12-16T23:21:30Z

README.md

+
+```js
+// The Multiaddr class has a factory method to help migration
+const { multiaddr } = require('multiaddr')


I think it would make more sense to have Multiaddr.parse(string) static and make constructor @private. That way this turns into

Suggested change

const { multiaddr } = require('multiaddr')

const { from: multiaddr } = require('multiaddr')

And if we really want to smooth out transition we could have deprecated alias multiaddr.

My argument for discouraging new Multiadddr is because has tendency to grow into new Multiaddr(any) over time.

My personal opinion is to reserve constructors for property initializations, all the computation and logic should go into dedicated functions.

Edit: Added more details in the contstructor

Gozala · 2020-12-16T23:23:37Z

package.json

    "test:browser": "aegir test -t browser",
-    "test:types": "npx tsc",
+    "test:types": "aegir ts -p check",


Suggested change

"test:types": "aegir ts -p check",

"test:types": "aegir ts -p check",

"prepare": "aegir ts -p types",

Gozala · 2020-12-16T23:25:32Z

src/codec.js

 function tuplesToStringTuples (tuples) {
  return tuples.map(tup => {
    const proto = protoFromTuple(tup)
-    if (tup.length > 1) {
+    if (tup.length > 1 && tup[1]) {


Checking length is redundant

Suggested change

if (tup.length > 1 && tup[1]) {

if (tup[1]) {

Gozala · 2020-12-16T23:49:16Z

src/index.js

+   * // <Multiaddr 047f000001060fa1 - /ip4/127.0.0.1/tcp/4001>
+   * ```
+   */
+  constructor (addr) {


Can we use this breaking change as an opportunity to make it more type friendly and switch to more cleaner approach:

const symbol = Symbol.for('@multiformats/js-multiaddr/multiaddr') class Multiaddr { /** * Parses string encoded multiaddr. * * @param {string} addr * @returns {Multiaddr} */ static parse(addr) { if (addr.length > 0 && addr.charAt(0) !== '/') { throw new Error(`multiaddr "${addr}" must start with a "/"`) } return new Multiaddr(codec.fromString(addr)) } /** * Decodedes binary encoded multiaddr. * * @param {Uint8Array} addr * @returns {Multiaddr} */ static decode(addr) { return new Multiaddr(codec.fromBytes(addr)) } static empty() { return Multiaddr.parse('') } /** * @typedef {Multiaddr|string|InstanceType<typeof String>|Uint8Array|null} ToMultiaddr * @param {ToMultiaddr} addr * @returns {Multiaddr} */ static from(addr) { if (typeof addr === 'string') { return Multiaddr.parse(addr) } else if (addr instanceof Uint8Array) { return Multiaddr.decode(addr) } else if (Multiaddr.isMultiaddr(addr)) { return new Multiaddr(addr.bytes) } else if (addr === null) { return Multiaddr.empty() } else { throw new Error('addr must be a string, Buffer, or another Multiaddr') } } /** * @param {any} value * @returns {value is Multiaddr} */ static isMultiaddr(value) { return Boolean(value && value[symbol]) } /** * @private * @param {Uint8Array} bytes */ constructor(bytes) { this.bytes = bytes } }

Which has bunch of benifs:

When you read Multiadddr.parse(input) it is clear what the input without any tooling or side trips.

Constructor is dead simple

Each code path is also absolutely clear.

You still have .from to take any input when you don't know what you have.

No added overhead on each instantiation when you know your types.

For what it's worth that is where multiformats/cid ended up and feedback so far had been positive.

Gozala · 2020-12-16T23:51:13Z

src/index.js

+   * ```
+   */
+  constructor (addr) {
+    Object.defineProperty(this, symbol, { value: true })


Can we turn that this into a getter please ?

class Multiaddr { get [symbol]() { return true } }

That is what we end up doing in other places.

Gozala · 2020-12-16T23:56:27Z

src/index.js

+    opts.host = parsed[2]
+    opts.transport = parsed[3]
+    opts.port = parseInt(parsed[4])
+    return opts


Can we do this instead, I find it a lot easier to follow (and engines can optimize better)

toOptions() { const [,family,host, transport, port] = this.toString().split('/') return { famility: family === 'ip4' ? 'ipv4' : 'ipv6', host, transport, port } }

Gozala · 2020-12-16T23:59:38Z

src/index.js

+   * Returns if something is a Multiaddr that is a name
+   *
+   * @param {Multiaddr} addr
+   * @returns {boolean} isName


Suggested change

* @returns {boolean} isName

* @returns {boolean}

Gozala · 2020-12-17T00:00:39Z

src/index.js

-   *   - how to achieve it in the browser?
+  /**
+   * Static factory method
+   *


I think it would make sense to deprecate it to encourage transition.

Suggested change

*

*

* @deprecated

hugomrdias requested review from jacobheun, vasco-santos and Gozala and removed request for jacobheun November 24, 2020 17:50

hugomrdias mentioned this pull request Nov 24, 2020

Track: Typescript types support ipfs/js-ipfs#2945

Closed

vasco-santos requested changes Nov 25, 2020

View reviewed changes

hugomrdias requested a review from vasco-santos November 26, 2020 12:02

vasco-santos reviewed Nov 26, 2020

View reviewed changes

src/index.js Show resolved Hide resolved

vasco-santos mentioned this pull request Nov 26, 2020

No documentation for getPeerId() #161

Closed

vasco-santos mentioned this pull request Dec 9, 2020

chore: add typedefs libp2p/js-libp2p#802

Merged

1 task

hugomrdias requested a review from vasco-santos December 9, 2020 11:24

hugomrdias added 18 commits December 15, 2020 12:20

feat: use github ci

678341b

fix: remove old docs stuff

8bf9cae

fix: fix resolver exports

708292c

fix: add some types to codec.js

4cec873

fix: add some types to convert

449065d

BREAKING CHANGE: Convert.toString now always a string

fix: add some types to protocols

7251ac9

fix: remove class-is and fix types in index.js

9a853c2

fix: transform multiaddr into a class

a09e829

BREAKING CHANGE: index.js default export no longer is both callable and constructor. encapsulate and decapsulate only accept Multiaddr instances as input.

fix: update package.json and tsconfig

04e0b48

chore: update readme to look like the other multiformats repos

980973d

fix: add more md code blocks to the examples

df5ab2a

fix: remove static resolve

64d1aed

chore: remove static resolve test

f30a721

fix: protocols type

f5cb727

fix: resolver types

26bf0ee

fix: update aegir

c83f1c1

fix: fix stringTuples example

89966e5

feat: export constructor

89766b0

BREAKING CHANGE: module export the Multiaddr class instead of a factory function.

fix: add type guard and mirgation docs

50719b3

hugomrdias force-pushed the feat/ts-types branch from 2d1c612 to 50719b3 Compare December 15, 2020 12:20

Gozala requested changes Dec 17, 2020

View reviewed changes

vasco-santos mentioned this pull request Jan 7, 2021

Minor fixes (types and validation) #168

Closed

vasco-santos mentioned this pull request Mar 15, 2021

Proposal: TypeScript Definitions for core libraries protocol/web3-dev-team#79

Merged

hugomrdias mentioned this pull request Mar 31, 2021

feat: add types #189

Merged

vasco-santos closed this in #189 Apr 8, 2021

achingbrain deleted the feat/ts-types branch November 30, 2023 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: integrate with aegir types cmd #159

fix: integrate with aegir types cmd #159

hugomrdias commented Nov 24, 2020 •

edited

Loading

vasco-santos Nov 25, 2020

hugomrdias Nov 26, 2020

vasco-santos Nov 26, 2020

hugomrdias Nov 26, 2020 •

edited

Loading

hugomrdias Nov 26, 2020

vasco-santos Nov 26, 2020

hugomrdias Nov 26, 2020

jacobheun Nov 30, 2020

vasco-santos left a comment

hugomrdias commented Dec 15, 2020

Gozala left a comment

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 16, 2020

Gozala Dec 17, 2020

	- uses: gozala/[email protected].4
	- uses: gozala/[email protected].8

	const { multiaddr } = require('multiaddr')
	const { from: multiaddr } = require('multiaddr')

	"test:types": "aegir ts -p check",
	"test:types": "aegir ts -p check",
	"prepare": "aegir ts -p types",

fix: integrate with aegir types cmd #159

fix: integrate with aegir types cmd #159

Conversation

hugomrdias commented Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugomrdias Nov 26, 2020 • 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

vasco-santos left a comment

Choose a reason for hiding this comment

hugomrdias commented Dec 15, 2020

Gozala 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

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

hugomrdias commented Nov 24, 2020 •

edited

Loading

hugomrdias Nov 26, 2020 •

edited

Loading