WebRTC transport

[omphalos]

Continuing the conversation from here.

Personally, I'm not fond of the places where I see this.Contact = .... It feels less than idiomatic to assign a constructor as a member of an instance. Perhaps we could avoid this.

Yeah, I agree with you. I'm thinking just a createContact function on the transport would be better.

Right now the Transport._send method takes a port, address. Maybe it would be better to pass it a contact instead and let the transport handle checking if it's the right type of contact.

Good point.

Another option is to leave Contact with IP and port and use the PeerJS server for those values and simply overload the method that assigns the nodeID.

I think that's a good suggestion and would simplify things a bit.

[omphalos]

I fixed my broken test. It took me a while to track down the source of my problem (I had copy-pasted the TCP integration tests but bucketIndex wasn't lining up in the WebRTC tests because it uses a different Contact and thus a different hash for nodeID.)

I still need to:

• Add argument asserts per the coding convention in this repo.
• Implement a thorough test suite for the address-port-contact, webrtc-contact, and webrtc files.
• Clean up my commit history.

Right now I'm just using EventEmitter as a pluggable stub for a signalling server. While convenient, I'm not sure if that's ideal. Also, the wrtc package doesn't compile on all platforms so I wonder if it's not better to make the WebRTC transport its own module.

[omphalos]

I fixed my broken test. It took me a while to track down the source of my problem (I had copy-pasted the TCP integration tests but bucketIndex wasn't lining up in the WebRTC tests because it uses a different Contact and thus a different hash for nodeID.)

I still need to:

• Add argument asserts per the coding convention in this repo.
• Implement a thorough test suite for the address-port-contact, webrtc-contact, and webrtc files.
• Clean up my commit history.

Right now I'm just using EventEmitter as a pluggable stub for a signalling server. While convenient, I'm not sure if that's ideal. Also, the wrtc package doesn't compile on all platforms so I wonder if it's not better to make the WebRTC transport its own module.

[bookchin]

Looking great! Keep up the awesome work. I'll proactively start reviewing and make some comments.

[bookchin]

Looking great! Keep up the awesome work. I'll proactively start reviewing and make some comments.

[bookchin]

On which platforms does the wrtc package fail to compile?

[bookchin]

On which platforms does the wrtc package fail to compile?

[omphalos]

Here's a discussion about the webtorrent project deciding to remove their wrtc dependency: webtorrent/webtorrent#303. It looks like it fails to compile on Windows and some Linux variants.

[omphalos]

Here's a discussion about the webtorrent project deciding to remove their wrtc dependency: webtorrent/webtorrent#303. It looks like it fails to compile on Windows and some Linux variants.

[omphalos]

@gordonwritescode Thanks for the feedback! I'll make that _createInstance change along with the other things I mentioned (asserts, tests, and rewriting my commits).

[omphalos]

@gordonwritescode Thanks for the feedback! I'll make that _createInstance change along with the other things I mentioned (asserts, tests, and rewriting my commits).

[omphalos]

The tests pass for me; I have to investigate why that one is failing on travis.

[omphalos]

The tests pass for me; I have to investigate why that one is failing on travis.

[bookchin]

That's strange. I can investigate as well. Thanks for all your hard work on this!

[bookchin]

That's strange. I can investigate as well. Thanks for all your hard work on this!

[bookchin]

Let's add an example page/script to a new examples/ directory too!

[bookchin]

Let's add an example page/script to a new examples/ directory too!

[bookchin]

Let me know when you think this is ready to merge. :)

[bookchin]

Let me know when you think this is ready to merge. :)

[bookchin]

Must be a race condition, running the build again passes. I suspect this issue already existed.

[bookchin]

Must be a race condition, running the build again passes. I suspect this issue already existed.

[omphalos]

@gordonwritescode Thanks again for taking a look. I'll do a little more clean up. Regarding new EventEmitter - sorry about that, I try to follow the coding style of what repo I work in and I should have looked more closely at that one! I'll also come with an example too. Accordingly I'll update the README.

Also, I'm not sure if you noticed, but the WebRTC transport doesn't emit a ready event (since it's ready right away). The other two transports do; it's not used anywhere in the code but I wanted to bring it up. If we wanted to do it with webrtc.js I think I would have to do it in a setImmediate call or something (otherwise it's pointless).

[omphalos]

@gordonwritescode Thanks again for taking a look. I'll do a little more clean up. Regarding new EventEmitter - sorry about that, I try to follow the coding style of what repo I work in and I should have looked more closely at that one! I'll also come with an example too. Accordingly I'll update the README.

Also, I'm not sure if you noticed, but the WebRTC transport doesn't emit a ready event (since it's ready right away). The other two transports do; it's not used anywhere in the code but I wanted to bring it up. If we wanted to do it with webrtc.js I think I would have to do it in a setImmediate call or something (otherwise it's pointless).

[bookchin]

Hmmm, yeah for now I'd emit on a setImmediate for the ready event. I can't remember exactly why I decided to emit that from the other transports, but I'd rather follow the same API and we can rip it out later if need be.

Super excited to see this merged, what a great contribution!

[bookchin]

Hmmm, yeah for now I'd emit on a setImmediate for the ready event. I can't remember exactly why I decided to emit that from the other transports, but I'd rather follow the same API and we can rip it out later if need be.

Super excited to see this merged, what a great contribution!

[omphalos]

Yeah, I'm excited too. Honestly the codebase makes it really easy to contribute to; I found all the code to be simple and very clean, practically pristine. It's one of the things that attracted me to this project.

Regarding the wrtc dependency, one idea is to keep it out of package.json so it doesn't trip people up and install it separately as step in travis. And maybe add a note to the README about this step in case someone comes by and decides they want to run the tests, since it's a little out of the ordinary. I like devDependencies for this but I think most people install those by default with npm install.

[omphalos]

Yeah, I'm excited too. Honestly the codebase makes it really easy to contribute to; I found all the code to be simple and very clean, practically pristine. It's one of the things that attracted me to this project.

Regarding the wrtc dependency, one idea is to keep it out of package.json so it doesn't trip people up and install it separately as step in travis. And maybe add a note to the README about this step in case someone comes by and decides they want to run the tests, since it's a little out of the ordinary. I like devDependencies for this but I think most people install those by default with npm install.

[omphalos]

I still have to test the examples but wanted to get the initial version out of the way.

[omphalos]

I still have to test the examples but wanted to get the initial version out of the way.

[bookchin]

Thanks for the kind words on code quality!

Instead of devDependencies, let's use optionalDependencies. That way folks can install with:

npm install --no-optional

[bookchin]

Thanks for the kind words on code quality!

Instead of devDependencies, let's use optionalDependencies. That way folks can install with:

npm install --no-optional

[jimlyndon]

Nice. @changetip give a coffee for @omphalos giving my browser dhts.

[jimlyndon]

Nice. @changetip give a coffee for @omphalos giving my browser dhts.

[changetip]

Hi @omphalos, I've delivered a tip worth a coffee (6,397 bits/$1.50) from @jimlyndon to your ChangeTip pocket.

Learn about ChangeTip

[changetip]

Hi @omphalos, I've delivered a tip worth a coffee (6,397 bits/$1.50) from @jimlyndon to your ChangeTip pocket.

Learn about ChangeTip

[omphalos]

Thanks @jimlyndon! Coffee and beer are two of my favorite things :)

I need to give the examples some more TLC as they're not functional in their current state.

[omphalos]

Thanks @jimlyndon! Coffee and beer are two of my favorite things :)

I need to give the examples some more TLC as they're not functional in their current state.

[omphalos]

Kind of stuck on this issue at the moment.

[omphalos]

Kind of stuck on this issue at the moment.

[bookchin]

Aw man, I wish I could help. I'm on Debian. I'll poke some of my Mac friends.

[bookchin]

Aw man, I wish I could help. I'm on Debian. I'll poke some of my Mac friends.

[bookchin]

@martindale if you have a chance could you try installing on OSX and see if you get the same compilation problem for wrtc?

[bookchin]

@martindale if you have a chance could you try installing on OSX and see if you get the same compilation problem for wrtc?

[bookchin]

@omphalos I re-ran the Travis build to see if the EADDRINUSE error was a race condition. Looks like it's new. Perhaps some of your tests use ports that are already bound?

[bookchin]

@omphalos I re-ran the Travis build to see if the EADDRINUSE error was a race condition. Looks like it's new. Perhaps some of your tests use ports that are already bound?

[martindale]

Fortunately, I've rid myself of OSX, back to Linux for me -- maybe @unusualbob can help?

[martindale]

Fortunately, I've rid myself of OSX, back to Linux for me -- maybe @unusualbob can help?

[omphalos]

@gordonwritescode Regarding the tests failures, the most questionable thing I did in this batch of changes (I think) was adding a setTimeout to WebRTCTransport#_send. I did this to get one of the eamples to work properly. Unfortunately I can't test my theory that this is the cause very easily. I guess I could use travis to run and debug the tests for me, but that sounds painful.

Also, I tried running the Dockerfile (Debian) but that fails as well for me.

[omphalos]

@gordonwritescode Regarding the tests failures, the most questionable thing I did in this batch of changes (I think) was adding a setTimeout to WebRTCTransport#_send. I did this to get one of the eamples to work properly. Unfortunately I can't test my theory that this is the cause very easily. I guess I could use travis to run and debug the tests for me, but that sounds painful.

Also, I tried running the Dockerfile (Debian) but that fails as well for me.

[omphalos]

I was able to work around this by rewriting the Dockerfile and using an old tag, so I'm not stuck any more. I'll take a look at the EADDRINUSE stuff before going further with the examples.

[omphalos]

I was able to work around this by rewriting the Dockerfile and using an old tag, so I'm not stuck any more. I'll take a look at the EADDRINUSE stuff before going further with the examples.

[bookchin]

@changetip send @omphalos some 1337 h4x0rz to help him finish!

[bookchin]

@changetip send @omphalos some 1337 h4x0rz to help him finish!

[changetip]

Hi @omphalos, I've delivered a tip worth 1 1337 h4x0rz from @gordonwritescode to your ChangeTip pocket.

Learn about ChangeTip

[changetip]

Hi @omphalos, I've delivered a tip worth 1 1337 h4x0rz from @gordonwritescode to your ChangeTip pocket.

Learn about ChangeTip

[omphalos]

Haha, alright .. hopefully over the next few days I will make some headway.

[omphalos]

Haha, alright .. hopefully over the next few days I will make some headway.

[omphalos]

@gordonwritescode I'm thinking EADDRINUSE is due to a change on travis' side. When I build master on travis, I see the same error.

[omphalos]

@gordonwritescode I'm thinking EADDRINUSE is due to a change on travis' side. When I build master on travis, I see the same error.

[omphalos]

Along these lines, I can't repro the issue when I run the tests myself (from inside docker).

[omphalos]

Along these lines, I can't repro the issue when I run the tests myself (from inside docker).

[bookchin]

I had a feeling. I'll fix the tests! Otherwise is this ready to merge?

[bookchin]

I had a feeling. I'll fix the tests! Otherwise is this ready to merge?

[omphalos]

@gordonwritescode Sorry, not ready yet. I have to wrap these examples up. I have time this weekend to work on this stuff so hopefully done real soon.

[omphalos]

@gordonwritescode Sorry, not ready yet. I have to wrap these examples up. I have time this weekend to work on this stuff so hopefully done real soon.

[omphalos]

@gordonwritescode I believe this is all set.

[omphalos]

@gordonwritescode I believe this is all set.

[omphalos]

@gordonwritescode just a heads up - I created a repo for the localStorage version of the storage option for the webRTC examples so it can be reused by folks in the future. Hope that sounds alright.

[omphalos]

@gordonwritescode just a heads up - I created a repo for the localStorage version of the storage option for the webRTC examples so it can be reused by folks in the future. Hope that sounds alright.

[bookchin]

@omphalos that's awesome! this is such a great addition to this project. merging now and will bump version and publish to npm shortly. thanks for all the hard work! @changetip celebrate!

[bookchin]

@omphalos that's awesome! this is such a great addition to this project. merging now and will bump version and publish to npm shortly. thanks for all the hard work! @changetip celebrate!

[changetip]

Hi @omphalos, I've delivered a tip worth 1 celebrate (40,594 bits/$10.00) from @gordonwritescode to your ChangeTip pocket.

Learn about ChangeTip

[changetip]

Hi @omphalos, I've delivered a tip worth 1 celebrate (40,594 bits/$10.00) from @gordonwritescode to your ChangeTip pocket.

Learn about ChangeTip

[bookchin]

Okay @omphalos, v0.6.0 is published to NPM and tagged. I've also added you as a collaborator to this repo. :) Cheers!

[bookchin]

Okay @omphalos, v0.6.0 is published to NPM and tagged. I've also added you as a collaborator to this repo. :) Cheers!

[omphalos]

Great, thanks! 👍

[omphalos]

Great, thanks! 👍