Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

Review of WebSocket Star #16

Merged
merged 12 commits into from
Sep 11, 2017
Merged

Review of WebSocket Star #16

merged 12 commits into from
Sep 11, 2017

Conversation

daviddias
Copy link
Member

@daviddias daviddias commented Sep 8, 2017

WIP, pushing as I go :)

package.json Outdated
@@ -4,13 +4,13 @@
"description": "libp2p-webrtc-star without webrtc. Just plain socket.io.",
"main": "src/index.js",
"bin": {
"websockets-star": "src/rendezvous/bin.js",
"websockets-star": "src/rendezvous/bin.js"
Copy link
Member

Choose a reason for hiding this comment

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

What about #11 ?

Copy link
Member Author

@daviddias daviddias Sep 8, 2017

Choose a reason for hiding this comment

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

answered in the issue #11 (comment)

package.json Outdated
@@ -32,13 +32,18 @@
"async": "^2.5.0",
"data-queue": "0.0.2",
"debug": "^3.0.1",
"hapi": "^16.5.2",
Copy link
Member

@mkg20001 mkg20001 Sep 8, 2017

Choose a reason for hiding this comment

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

Can't we have those modules split up in client and server? Putting hapi and socket.io into this module only makes the client 10x bigger without any benefits for the user.
Edit: That's what #11 was about

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, we can do that then.

Copy link
Member Author

Choose a reason for hiding this comment

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

@daviddias
Copy link
Member Author

I won't have time to look into this again until tomorrow. @mkg20001 if you want to take a stab at continuing modularizing and making sure everything is wired together now that the rendezvous service was factor out, be my guest :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants