-
Notifications
You must be signed in to change notification settings - Fork 19
Conversation
src/socket-to-conn.ts
Outdated
@@ -49,11 +63,11 @@ module.exports = (socket, options = {}) => { | |||
: undefined, | |||
|
|||
// If the remote address was passed, use it - it may have the peer ID encapsulated | |||
remoteAddr: options.remoteAddr, | |||
remoteAddr: options.remoteAddr ?? toMultiaddr(socket.remoteAddress ?? '', socket.remotePort ?? ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add this conversion because remoteAddr
is required in MultiaddrConnection
interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just throw if options.remoteAddr
or socket.remoteAddress
or socket.remotePort
is nullish. Having a blank multiaddr or ill-formed multiaddr is not useful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made remoteAddr
option mandatory because all usages of ToConnectionOptions
specify it. Please verify.
src/listener.ts
Outdated
}) | ||
|
||
const maConn = toMultiaddrConnection(channel, { | ||
remoteAddr: ipPortToMultiaddr(req.socket.remoteAddress, req.socket.remotePort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw when req.socket.remoteAddress
is undefined? It can be, according to the docs https://nodejs.org/api/net.html#class-netsocket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's necessary or if its just used for diagnostics, @vasco-santos any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made remoteAddr
option mandatory because all usages of ToConnectionOptions
specify it. Please verify.
src/index.ts
Outdated
const log = logger('libp2p:webrtcdirect') | ||
|
||
export interface WebRTCDirectListenerOptions extends ListenerOptions{ | ||
channelOptions?: Object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the same logic as in js-libp2p-webrtc-star/webrtc-star-transport. However I didn't describe the type of channelOptions
. As far as this code is concerned, Object
type looked fine but let me know if you want to describe it thoroughly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to describe the shape entirely, not use Object
type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look. The exact shape is described here https://www.npmjs.com/package/libp2p-webrtc-peer#api. See constructor options:
{
initiator: false,
channelConfig: {},
channelName: '<random string>',
config: { iceServers: [{ urls: 'stun:stun.l.google.com:19302' }, { urls: 'stun:global.stun.twilio.com:3478?transport=udp' }] },
offerOptions: {},
answerOptions: {},
sdpTransform: function (sdp) { return sdp },
stream: false,
streams: [],
trickle: true,
allowHalfTrickle: false,
wrtc: {}, // RTCPeerConnection/RTCSessionDescription/RTCIceCandidate
objectMode: false
}
I described all the keys of this object. However for config
key I used a plain object. Do you want me to describe it more rigid with iceServers
and urls
?
@vasco-santos @wemeetagain , please look when you have a minute. |
- make `ToConnectionOptions` mandatory in `toMultiaddrConnection` - make `remoteAddr` mandatory in `ToConnectionOptions`
- Update code to use interfaces from `@libp2p/interfaces` - Update module name to use the `@libp2p/` scope - Use the same `@libp2p/webrtc-peer` module as `@libp2p/webrtc-star` - Update licenses, CI and project config to match other `@libp2p/` modules BREAKING CHANGE: this module now only exports named exports and is ESM only
I've updated this PR to use the new It also has auto-release now, should be all set up to go. |
GH actions is being really flaky today, only seems to run on about 1/2 of the commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
## [1.0.0](v0.7.1...v1.0.0) (2022-03-28) ### ⚠ BREAKING CHANGES * this module is now ESM-only Co-authored-by: achingbrain <alex@achingbrain.net> ### Features * convert to typescript ([#151](#151)) ([85ce5cf](85ce5cf)) ### Bug Fixes * add 'node-pre-gyp' installation to 'check' and 'test-node' actions ([#152](#152)) ([bf4a68b](bf4a68b)) ### Trivial Changes * add note about `node-pre-gyp` to readme.md ([#141](#141)) ([ab4cc82](ab4cc82)), closes [#140](#140) * **deps-dev:** bump aegir from 35.2.1 to 36.0.0 ([#139](#139)) ([720cfad](720cfad)) * replace Travis with Github Actions ([#150](#150)) ([a73735b](a73735b)) * update project config ([13ab340](13ab340)) * update Readme ([#148](#148)) ([ba9facb](ba9facb))
🎉 This PR is included in version 1.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The PR is not ready for a merge. I've pushed it to ask some questions. The questions are posted as comments at the relevant source lines.