-
Notifications
You must be signed in to change notification settings - Fork 333
add DHT invites, only as a client #1246
Conversation
| .use(require('ssb-room/tunnel/client')) | ||
| // .use(require('ssb-dht-invite')) // this one must come before dhtTransport | ||
| // .use(dhtTransport) | ||
| .use(require('ssb-dht-invite')) |
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 always wondered why the chaining order is like it is here as it is not arbitrary, right? It would be really great to add some documentation about that here at some point.
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's not arbitrary, but it's also not documented. To be candid, I think the SecretStack plugin system is an anti-pattern that encourages global mutable state. I'm doing my best to randomize the plugin order in Oasis with this pattern (https://github.com/fraction/flotilla/blob/master/index.js) but it's still a mess. I really wish these modules would just require() their dependencies instead of using another dependency system.
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.
To be candid, I think the SecretStack plugin system is an anti-pattern that encourages global mutable state.
😱 hot take! I actually like the secret-stack plugin system, out of all the other ideas in the SSB stack.
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.
If y'all want some insight on what depends on what, I wrote it down for Manyverse here:
|
@staltz Is this fine to merge? I'm not worried about whether it works super well, even 10% reliability is better than 0%. |
|
@christianbundy Yes. Widespread testing would help us know, I only did a local test here with my few devices (all under the same network, or in the same city cellular network). This PR needs a git conflict resolution before merging though |
7bd491f to
fe41b67
Compare
After debugging in #1277, it seems that our travis cache got corrupted. This simply puts a cache verification step in front of the build process. If the caching becomes a problem now, we shoul at least get some clear info on what's happening. Also, this disables explicit caching of ~/.npm, because that is handled implicitly by travis these days.
8e259ba to
d3826fa
Compare
Implemented! But I think we need some thorough testing before merging. I myself was not able to connect Patchwork to Manyverse via DHT invites. Sometimes it depends on the NAT gods of the internet.
And note that because Patchwork doesn't display "connecting" states, only "connected" states, as soon as you press "accept invite", the UI will give you no indication that the invite was converted into a "connecting" status, so the user might feel confused that nothing is going on. Hopefully someone else could chime in and do something about that. Feel free to update this branch with new commits