callbacks -> async / await

[dirkmc]

BREAKING CHANGE: All places in the API that used callbacks are now replaced with async/await

[alanshaw]

Sorry I meant to request changes 😂 I do approve, but just needs some minor tweaks!

[vasco-santos]

Great @dirkmc !

Just left a comment regarding the listener.close. Other than that, can you please update the API on the README?

[vasco-santos]

Awesome! :D

Just noticed a minor thing

[jacobheun]

Perhaps this should be a follow up PR instead, but since we're doing a breaking change it might be worth making listen support an array of multiaddrs now https://github.com/libp2p/interface-transport/issues/41.

[dirkmc]

Makes sense... should listen require an array, or should it also support single multiaddr?

[jacobheun]

I think it makes sense for it to just support an array. In the majority of cases the switch is going to be handling the .listen call, so most users shouldn't notice the change.

[vasco-santos]

I agree that we can leverage this breaking change and change the multiaddr param to an array. Should we do this in a separate PR, but only release a new version of interface-transport once that is done?

[jacobheun]

@dirkmc I'm holding on the release until we've added the listening array support. Are you working on that? If not I can start it, just avoiding the potential duplicate effort :)

[dirkmc]

@jacobheun please go ahead - I have some other things going on at the moment so only able to snatch a bit of time here and there to work

[jacobheun]

This looks good. One thing I would recommend changing are the trailing semicolons to leading semicolons.

listener.on('connection', async (conn) => {
  // ...
});

(async () => {
  // ...
})()

vs

listener.on('connection', async (conn) => {
  // ...
})

;(async () => {
  // ...
})()

The main reasoning being clarity. It's easier to reason that the leading semicolon belongs to that block and has the purpose of breaking up statements that would otherwise clash. Trailing commas are easier to misinterpret as being unneeded and subsequently removed.

Linting is smart enough to recognize this as not being an extra comma, but it would be easy for a human to accidentally remove it. Since we have no local test suite for this, that can be a rather annoying mistake to discover after a release.

[dirkmc]

You're right that's clearer. Fixed

[alanshaw]

Since this PR was created @dirkmc and I talked about dial and decided that it would be better for it to return a promise which resolves when the connection has been established (as per these docs libp2p/js-libp2p-websockets#82).

It's not strictly necessary, but I feel like it is a more intuitive API. @jacobheun is there some strong counterpoint to this?

Secondly, AbortError - both in node-fetch and natively in the browser the error thrown when you abort a fetch is an AbortError - a subclass of Error with a type property with the value "aborted".

I've altered abortable-iterator to return an error that looks like this (https://github.com/alanshaw/abortable-iterator/blob/master/AbortError.js) and have used the type property in tests to distinguish this error from other errors. There's probably some value in following this precedent (over our own 'ABORT_ERR' code) ...what do you think? That is not to say we can't add our own code!

[jacobheun]

Since this PR was created @dirkmc and I talked about dial and decided that it would be better for it to return a promise which resolves when the connection has been established (as per these docs libp2p/js-libp2p-websockets#82).

I am in favor of waiting to return the active connection or erroring. With the ability to abort we really have everything we need and I agree it's more intuitive.

Secondly, AbortError - both in node-fetch and natively in the browser the error thrown when you abort a fetch is an AbortError - a subclass of Error with a type property with the value "aborted".

We should comply with the standard and add type. I think we should add the code as well, as we are using that in errors everywhere else.

[alanshaw]

I add a code in abortable-iterator as well, but I used the string "ERR_ABORTED" (by default).

[dirkmc]

I think the standard error code is 'ABORT_ERR':
https://heycam.github.io/webidl/#aborterror

[alanshaw]

Ah, ok cool, I'll update abortable-iterator then 👍

Also, I just realised await dial IS what you implemented here I was looking at

interface-transport/src/dial-test.js

Line 59 in b30ee5f

const socket = transport.dial(addrs[0], { signal: controller.signal })

and didn't read further... 🙄