-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add properties and functions to client and server #1
feat: add properties and functions to client and server #1
Conversation
Note: tests are failing same way as already failing in master branch |
client.js
Outdated
@@ -7,12 +7,14 @@ var wsurl = require('./ws-url') | |||
|
|||
module.exports = function (addr, opts) { | |||
const location = typeof window === 'undefined' ? {} : window.location | |||
opts = opts || {} |
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'd move this to a default param instead of checking here. module.exports = function (addr, opts = {}) {
client.js
Outdated
|
||
const stream = duplex(socket, opts) | ||
stream.remoteAddress = url | ||
stream.socket = socket |
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.
Is this needed? Can we map stream.destroy
to socket.terminate
? This would give users the forceful close option.
server.js
Outdated
@@ -41,6 +41,9 @@ module.exports = !WebSocket.Server ? null : function (opts, onConnection) { | |||
wsServer.on('connection', function (socket, req) { | |||
var stream = ws(socket) | |||
stream.remoteAddress = req.socket.remoteAddress | |||
stream.remotePort = req.socket.remotePort | |||
stream.close = () => emitter.close() |
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.
Shouldn't emitter.close()
be socket.close()
? emitter.close
is going to shut the whole server down, not close the connection.
I'd also map stream.destroy
to socket.terminate
for consistency with the returned client
streams.
d68d064
to
dcb13a6
Compare
dcb13a6
to
c78168d
Compare
In the context of libp2p/js-libp2p-websockets#92, as well as https://github.com/libp2p/interface-transport#multiaddrconnection, a few properties were added