Skip to content
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

Automatically download manifest from server #52

Merged
merged 5 commits into from
Jan 23, 2020
Merged

Conversation

christianbundy
Copy link
Contributor

Manifests are hard and the server is happy to provide one for us.

This simplifies ssb-client to remove the manifest option, using muxrpc's manifest bootstrap instead of asking the user or storing one at ~/.ssb/manifest.json.

I don't think we want to support any use-case where the client has a different manifest than the server, please correct me if I'm wrong.

client.js Show resolved Hide resolved
client.js Show resolved Hide resolved
@dominictarr
Copy link
Contributor

I don't think it's a good idea to completely remove this option, because it adds another round trip to establishing the connection. You might not notice that in CLI, but if you are accepting an invite code from some tiny pub running on a rpi behind dynamic dns or through tor, or on satellite internet, then latency becomes quite significant. ssb replication is carefully designed to minimize round trips!

If this was the default if a manifest wasn't provided, that would be okay, though.

@christianbundy
Copy link
Contributor Author

Sweet, I'll change this PR to re-add manifest back as an option.

@christianbundy
Copy link
Contributor Author

Fixed! I've also gone through the files I've changed and tried to make their style more consistent. I specifically left the visual whitespace changes that seemed intentional.

test/index.js Show resolved Hide resolved
client.js Show resolved Hide resolved
Copy link

@soapdog soapdog left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@christianbundy
Copy link
Contributor Author

Thanks @soapdog!

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

Successfully merging this pull request may close these issues.

3 participants