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

Add http daemon #22

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add http daemon #22

wants to merge 4 commits into from

Conversation

drbh
Copy link

@drbh drbh commented Aug 28, 2018

Copied files server.js add.js create.js get.js from 3c691c032a047e99237af24af6956b9068c314e6 branch of orbit-db-http-server at https://github.com/haadcode/orbit-db-http-server.git

Added a new file in commands that run the daemon. This currently runs in the foreground and can be canceled with CTL + C, also hard coded the port and host to remove unneeded files.

-added the files that the original PR was missing.

@RichardLitt
Copy link
Member

Continuation of #21. :)

@drbh
Copy link
Author

drbh commented Dec 5, 2018

bump

@aphelionz
Copy link
Member

Thanks for the bump @drbh. I'll be taking a closer look at this tomorrow.

@aphelionz
Copy link
Member

@drbh Would you mind either writing a positive test case or some unit test (which we'll need to merge anyway)?

Another question I have is if we could include the orbit-db-http-server package here instead of copying the files over. Maybe there is something we can do on our end to get that merged and usable for you.

I think adding an HTTP server is an extremely useful addition and besides just wanting to do it right I think we want to generalize this to be useful across all the orbit packages

@haadcode
Copy link
Member

Happy to move the orbit-db-http-server repo if you want to continue it. Agreed with @aphelionz that making it generic and usable in other projects too would be the best way forward.


/* Export as Yargs command */
exports.command = 'daemon'
exports.desc = 'Start a orbitdb daemon'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor grammar suggestion :)

Suggested change
exports.desc = 'Start a orbitdb daemon'
exports.desc = 'Start an orbitdb daemon'

const address = req.params[0]

// Get params on how we should output the results
const shouldStream = req.query.live || false
Copy link
Collaborator

Choose a reason for hiding this comment

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

A more concise way to do it

Suggested change
const shouldStream = req.query.live || false
const shouldStream = Boolean(req.query.live)

const db = await req.orbitdb.open(address, {
create: false,
sync: true,
localOnly: req.query.live ? !req.query.live : true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
localOnly: req.query.live ? !req.query.live : true,
localOnly: !Boolean(req.query.live),

Comment on lines +21 to +22
const defaultPort = 37373
const defaultHost = "localhost"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Standard constants can be written in all caps like below

const DEFAULT_PORT = 37373
const DEFAULT_HOST = "localhost"


return new Promise((resolve, reject) => {
// Start the HTTP server
let server = app.listen({ port: port, host: host }, async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let server = app.listen({ port: port, host: host }, async () => {
let server = app.listen({ port, host }, async () => {

}
const err = false
if (err){
reject(server)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this block of code ever execute? The err is set to false as always.

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.

5 participants