Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Migrate to pull-streams #10

Merged
merged 1 commit into from
Sep 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,6 @@ A valid (read: that follows this abstraction) connection, must implement the fol
- `conn.getObservedAddrs(callback)`
- `conn.getPeerInfo(callback)`
- `conn.setPeerInfo(peerInfo)`
- `conn.destroy`
- `conn.write`
- `conn.read`
- `conn.pipe`
- `conn.end`
- `conn.pause`
- `conn.resume`
- `conn.destroy`
- `...`

### Get the Observed Addresses of the peer in the other end
Expand Down
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"name": "interface-connection",
"version": "0.1.8",
"description": "A test suite and interface you can use to implement a connection interface.",
"main": "lib/index.js",
"main": "src/index.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • change before merge

"jsnext:main": "src/index.js",
"scripts": {
"lint": "aegir-lint",
Expand Down Expand Up @@ -30,8 +30,8 @@
},
"homepage": "https://github.com/diasdavid/interface-connection",
"dependencies": {
"duplexify": "diasdavid/duplexify#a22bcdf",
"timed-tape": "^0.1.1"
"pull-defer": "^0.2.2",
},
"devDependencies": {
"aegir": "^6.0.1"
Expand All @@ -40,4 +40,4 @@
"David Dias <daviddias.p@gmail.com>",
"Pau Ramon Revilla <masylum@gmail.com>"
]
}
}
68 changes: 36 additions & 32 deletions src/connection.js
Original file line number Diff line number Diff line change
@@ -1,56 +1,60 @@
'use strict'

const util = require('util')
const Duplexify = require('duplexify')
const defer = require('pull-defer/duplex')

module.exports = Connection
module.exports = class Connection {
constructor (conn, info) {
this.peerInfo = null
this.conn = defer()

util.inherits(Connection, Duplexify)

function Connection (conn) {
if (!(this instanceof Connection)) {
return new Connection(conn)
if (conn) {
this.setInnerConn(conn, info)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you like class, but we kind of loose the ability to create an instance without calling new.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that's a good thing in my opinion, calling a function and creating an instance should be two different things.

} else if (info) {
this.info = info
}
}

Duplexify.call(this)
get source () {
return this.conn.source
}

let peerInfo
get sink () {
return this.conn.sink
}

this.getPeerInfo = (callback) => {
if (conn && conn.getPeerInfo) {
return conn.getPeerInfo(callback)
getPeerInfo (callback) {
if (this.info && this.info.getPeerInfo) {
return this.info.getPeerInfo(callback)
}

if (!peerInfo) {
if (!this.peerInfo) {
return callback(new Error('Peer Info not set yet'))
}

callback(null, peerInfo)
callback(null, this.peerInfo)
}

this.setPeerInfo = (_peerInfo) => {
if (conn && conn.setPeerInfo) {
return conn.setPeerInfo(_peerInfo)
setPeerInfo (peerInfo) {
if (this.info && this.info.setPeerInfo) {
return this.info.setPeerInfo(peerInfo)
}
peerInfo = _peerInfo

this.peerInfo = peerInfo
}

this.getObservedAddrs = (callback) => {
if (conn && conn.getObservedAddrs) {
return conn.getObservedAddrs(callback)
getObservedAddrs (callback) {
if (this.info && this.info.getObservedAddrs) {
return this.info.getObservedAddrs(callback)
}
callback(null, [])
}

this.setInnerConn = (_conn) => {
conn = _conn
this.setReadable(conn)
this.setWritable(conn)
}

// .destroy is implemented by Duplexify

if (conn) {
this.setInnerConn(conn)
setInnerConn (conn, info) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to pass info as an argument? It should be able to get it through conn.getPeerInfo

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • info is not the peerInfo, it's another instance of Connection.
  • This is sometimes needed because the info data comes from one lower level Connection but the thing we are wrapping is something else. See libp2p-multistream for examples of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean things like: https://github.com/multiformats/js-multistream/pull/19/files#diff-0ef79b038534cac22e85e5b18ba0e546R49, this is because that pull-handshake doesn't yield the original conn on the .rest()

Kind of sad that we loose that clean abstraction. I would rather patch them (similar to the way we do it in stream muxing)

conn.setInnerConn(underlyingConn)
conn.getObservedAddrs = underlyingConn.getObservedAddrs
...

``

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really not possible to fix as far as I understand..rest() needs to be a different object to ensure things are working in the handshake setup.

Copy link
Member

@daviddias daviddias Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roger, what I mean is that I would love that conn.setInnerConn still only accepts one argument, and then if we have to do stiching, we do it after. Because it seems to be something specific to Multistream

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why it handles only one argument as well, so that in most cases you can just pass in the single one, and if you need to you can pass both.

this.conn.resolve(conn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this resolve method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.conn is a pull-defer stream. It is similar to what you can do with Duplexify

if (info) {
this.info = info
} else {
this.info = conn
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting the info to conn? This confused me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, calling it info is still confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

infoConn?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

infoConn (or baseConn) and a good comment in that function explaining why it exists, for now :)

}
}
}
3 changes: 1 addition & 2 deletions src/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
'use strict'

exports.connection = require('./connection.js')
exports.Connection = require('./connection.js')
exports.Connection = require('./connection')
2 changes: 2 additions & 0 deletions tests/base-test.js → test/base-test.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

module.exports.all = function (test, common) {
test('a test', function (t) {
common.setup(test, function (err, conn) {
Expand Down
2 changes: 2 additions & 0 deletions tests/index.js → test/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict'

var timed = require('timed-tape')

module.exports = function (test, common) {
Copy link
Member

@daviddias daviddias Aug 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking that our way to test connections can be a common that returns 'conn pairs' that are connected, so that every data flowing can be tested + we can do the wrapping tests. It would be a great addition.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to https://github.com/ipfs/interface-ipfs-core/blob/master/src/generic.js#L18-L30, where instead of returning an instance, it returns a thing that create instances, in this case, it expects to return 2 Connection instances that are connected (like pull-pair) so that tests can be run on top of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, once there are actual tests in here :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we can repurpose a bunch of libp2p-tcp tests to there :)

Expand Down
3 changes: 0 additions & 3 deletions test/test.spec.js

This file was deleted.